Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

set solc version of VRF V2 Plus related contracts to 0.8.19 #12479

Merged
merged 10 commits into from
Mar 20, 2024

Conversation

jinhoonbang
Copy link
Contributor

  • set solc version of VRF V2 Plus related contracts to 0.8.19
  • creates a new ChainSpecificUtil.sol that works with 0.8.19. The existing one only works with 0.8.6 and has been renamed accordingly.
  • gas savings:
  • requestRandomWords avg: 28367 median: 33044 max: 37844 -> avg: 28329 median: 32997 max: 37797
  • fulfillRandomWords avg: 127324 median: 97286 max: 231262 -> avg: 127173 median: 97062 max: 231146

Copy link
Contributor

I see you updated files related to core. Please run pnpm changeset in the root directory to add a changeset.

Copy link
Contributor

@leeyikjiun leeyikjiun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we just change the pragma solidity 0.8.6 to pragma solidity ^0.8.6 or pragma solidity ^0.8.19 instead of a strict pragma solidity 0.8.19. Just in case this happens again in the future, keep it more future proof.


import {ChainSpecificUtil} from "../../ChainSpecificUtil.sol";
import {ChainSpecificUtil} from "./libraries/ChainSpecificUtil.sol";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this used instead of import {ChainSpecificUtil} from "../ChainSpecificUtil_v0_8_6.sol"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reasoning here is that ../ChainSpecificUtil_v0_8_6.sol only works with 0.8.6 but here I am upgrading the version of blockhashstore to 0.8.19 which requires ./libraries/ChainSpecificUtil.sol


import {ChainSpecificUtil} from "../../ChainSpecificUtil.sol";
import {ChainSpecificUtil} from "./libraries/ChainSpecificUtil.sol";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as for comment above.

@@ -4,7 +4,7 @@ pragma solidity ^0.8.4;
import {BlockhashStoreInterface} from "../interfaces/BlockhashStoreInterface.sol";
import {VRF} from "../../vrf/VRF.sol";
import {VRFConsumerBaseV2Plus, IVRFMigratableConsumerV2Plus} from "./VRFConsumerBaseV2Plus.sol";
import {ChainSpecificUtil} from "../../ChainSpecificUtil.sol";
import {ChainSpecificUtil} from "./libraries/ChainSpecificUtil.sol";
Copy link
Contributor

@ibrajer ibrajer Mar 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will not repeat the question but you got the idea :) It applies to every occurrence of the same line of code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep :) let me know if the reasoning above makes sense

@jinhoonbang
Copy link
Contributor Author

Should we just change the pragma solidity 0.8.6 to pragma solidity ^0.8.6 or pragma solidity ^0.8.19 instead of a strict pragma solidity 0.8.19. Just in case this happens again in the future, keep it more future proof.

I think once we release v2.5, we will keep v2.5 at 0.8.19. Following the general guidelines here

Solidity contracts should have a pragma which is locked to a specific version.

Copy link
Contributor

Go solidity wrappers are out-of-date, regenerate them via the make wrappers-all command

Copy link
Contributor

Go solidity wrappers are out-of-date, regenerate them via the make wrappers-all command

ibrajer
ibrajer previously approved these changes Mar 20, 2024
Copy link
Contributor

@ibrajer ibrajer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, all comments and concerns have been addressed.


import {ArbSys} from "./vendor/@arbitrum/nitro-contracts/src/precompiles/ArbSys.sol";
import {ArbGasInfo} from "./vendor/@arbitrum/nitro-contracts/src/precompiles/ArbGasInfo.sol";
import {OVM_GasPriceOracle} from "./vendor/@eth-optimism/contracts/v0.8.6/contracts/L2/predeploys/OVM_GasPriceOracle.sol";
import {OVM_GasPriceOracle} from "./vendor/@eth-optimism/contracts/v0.8.9/contracts/L2/predeploys/OVM_GasPriceOracle.sol";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work with v0.8.19

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes OVM_GasPriceOracle works with anything above v0.8.9

@@ -1,4 +1,4 @@
pragma solidity 0.8.6;
pragma solidity 0.8.19;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these files need to be updated in this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not necessary but automations team agreed to it . https://chainlink-core.slack.com/archives/C033D4HSSQJ/p1710877172545099

@cl-sonarqube-production
Copy link

@jinhoonbang jinhoonbang added this pull request to the merge queue Mar 20, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 20, 2024
@jinhoonbang jinhoonbang added this pull request to the merge queue Mar 20, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 20, 2024
@jinhoonbang jinhoonbang added this pull request to the merge queue Mar 20, 2024
Merged via the queue into develop with commit 93762cc Mar 20, 2024
115 checks passed
@jinhoonbang jinhoonbang deleted the vrfv2plus-solc-0.8.19 branch March 20, 2024 20:06
kidambisrinivas pushed a commit that referenced this pull request Mar 27, 2024
* set solc version of VRF V2 Plus related contracts to 0.8.19

* bump solc version in hardhat config for v2.5 coordinator

* fix failing tests due to gas change

* auto detect solc version for vrf contracts

* upgrade mercury registry

* add missing file

* make generate
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants