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.

Sign up for GitHub

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

Jump to bottom

rename VRF V2 plus to V2_5. rename eth to native #10656

Merged
merged 10 commits into from
Sep 26, 2023
Merged

rename VRF V2 plus to V2_5. rename eth to native #10656

merged 10 commits into from
Sep 26, 2023

Conversation

jinhoonbang
Copy link
Contributor

@jinhoonbang jinhoonbang commented Sep 14, 2023 •
edited
Loading

  • rename VRFCoordinatorV2Plus to VRFCoordinatorV2_5
  • introduce VRFCoordinatorV2PlusInterfaceInternal which is used to generate gethwrappers that offchain can use across VRFV2_x versions
  • rename eth to native

@github-actions
Copy link
Contributor

I see that you haven't updated any CHANGELOG files. Would it make sense to do so?

@jinhoonbang jinhoonbang marked this pull request as ready for review September 15, 2023 04:50
@makramkd
Copy link
Contributor

Updating this PR w/ discussions we had:

  • Can rename everything to 2_5 in the contracts except for the wrapper since that might end up being upgraded as a regular consumer.
  • Can keep offchain code using the v2plus name and using interfaces instead of concrete types.

@jinhoonbang jinhoonbang force-pushed the VRF-603-rename-vrf-coordinator-v-2-plus-to-vrf-coordinator-v-2-5 branch from 6f0eea8 to 5d4ee24 Compare September 22, 2023 00:28
jinhoonbang
jinhoonbang commented Sep 22, 2023
View reviewed changes
contracts/src/v0.8/dev/interfaces/IVRFMigratableCoordinatorV2Plus.sol
@@ -1,35 +0,0 @@
// SPDX-License-Identifier: MIT
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced by IVRFCoordinatorV2Plus.sol

jinhoonbang
jinhoonbang commented Sep 22, 2023
View reviewed changes
core/services/blockhashstore/delegate.go
@@ -13,7 +13,7 @@ import (
v1 "github.com/smartcontractkit/chainlink/v2/core/gethwrappers/generated/solidity_vrf_coordinator_interface"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

as of now, we don't have a way to specify multiple coordinator v2 plus addresses. After introducing v2_6, we will need BHS feeder to monitor both 2_5 and 2_6until2_5` is deprecated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

similar story for vrf jobs.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could just create new BHS jobs w/ the new coordinator address right?

@github-actions
Copy link
Contributor

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

1 similar comment
@github-actions
Copy link
Contributor

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

makramkd
makramkd reviewed Sep 22, 2023
View reviewed changes
contracts/src/v0.8/dev/interfaces/IVRFCoordinatorV2_5.sol Outdated Show resolved Hide resolved
contracts/src/v0.8/dev/interfaces/IVRFCoordinatorV2_5_Migration.sol Outdated Show resolved Hide resolved
contracts/scripts/native_solc_compile_all_vrf Outdated Show resolved Hide resolved
contracts/src/v0.8/dev/interfaces/IVRFMigratableConsumerV2_5.sol Outdated Show resolved Hide resolved
contracts/src/v0.8/dev/interfaces/IVRFSubscriptionV2_5.sol Outdated Show resolved Hide resolved
contracts/src/v0.8/dev/vrf/SubscriptionAPI.sol
event EthFundsRecovered(address to, uint256 amount);
event NativeFundsRecovered(address to, uint256 amount);
Copy link
Contributor

Choose a reason for hiding this comment

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

With this change here, does the ABI in atlas need to be updated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah we need to update ABI and Benthos pipelines

contracts/src/v0.8/dev/vrf/SubscriptionAPI.sol
event SubscriptionFundedWithEth(uint256 indexed subId, uint256 oldEthBalance, uint256 newEthBalance);
event SubscriptionFundedWithNative(uint256 indexed subId, uint256 oldNativeBalance, uint256 newNativeBalance);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is indexed in atlas so we def need to update the ABI there, perhaps a follow up task.

contracts/src/v0.8/dev/vrf/libraries/VRFV2_5_Client.sol Outdated Show resolved Hide resolved
core/services/blockhashstore/delegate.go
@@ -13,7 +13,7 @@ import (
v1 "github.com/smartcontractkit/chainlink/v2/core/gethwrappers/generated/solidity_vrf_coordinator_interface"
Copy link
Contributor

Choose a reason for hiding this comment

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

We could just create new BHS jobs w/ the new coordinator address right?

@makramkd
Copy link
Contributor

Lots of my comments above are prior to hashing things out regarding naming this week, can ignore.

makramkd
makramkd previously approved these changes Sep 22, 2023
View reviewed changes
Copy link
Contributor

@makramkd makramkd left a comment

Choose a reason for hiding this comment

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

💪 This is a pretty Herculean rename PR.

@makramkd
Copy link
Contributor

Ah damn looks like the migration smoke test is failing: https://github.com/smartcontractkit/chainlink/actions/runs/6269048139/job/17024994184?pr=10656

@cl-sonarqube-production
Copy link

SonarQube Quality Gate

Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 16 Code Smells

95.9% 95.9% Coverage
0.0% 0.0% Duplication

@jinhoonbang jinhoonbang added this pull request to the merge queue Sep 25, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 25, 2023
@jinhoonbang jinhoonbang added this pull request to the merge queue Sep 25, 2023
@jinhoonbang jinhoonbang removed this pull request from the merge queue due to a manual request Sep 25, 2023
@jinhoonbang jinhoonbang added this pull request to the merge queue Sep 25, 2023
Merged via the queue into develop with commit a9a5120 Sep 26, 2023
@jinhoonbang jinhoonbang deleted the VRF-603-rename-vrf-coordinator-v-2-plus-to-vrf-coordinator-v-2-5 branch September 26, 2023 00:22
@snehaagni snehaagni mentioned this pull request Oct 18, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@makramkd makramkd makramkd approved these changes

@se3000 se3000 Awaiting requested review from se3000

@connorwstein connorwstein Awaiting requested review from connorwstein

@jkongie jkongie Awaiting requested review from jkongie

@prashantkumar1982 prashantkumar1982 Awaiting requested review from prashantkumar1982

@samsondav samsondav Awaiting requested review from samsondav

@jmank88 jmank88 Awaiting requested review from jmank88

@kidambisrinivas kidambisrinivas Awaiting requested review from kidambisrinivas

@iljapavlovs iljapavlovs Awaiting requested review from iljapavlovs

@vreff vreff Awaiting requested review from vreff

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jinhoonbang @makramkd