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

feature: eth-vrf #9646

Merged
merged 43 commits into from
Jul 14, 2023
Merged

feature: eth-vrf #9646

merged 43 commits into from
Jul 14, 2023

Conversation

makramkd
Copy link
Contributor

@makramkd makramkd commented Jun 19, 2023

feature set:

  • add a subscription owner contract that allows users to migrate across coordinators
  • add a new "v2.5" contract that supports link and native billing. this has breaks the abi in small ways, but nothing too major.
  • the v2.5 contract also informs the consumer how much they paid by writing to storage. optionally, we can call back into their contract again.
  • add a new base consumer contract to account for v2.5 behavior

other changes:

  • refactor the vrf package to have v1, v2 subpackages (among others) to ease the addition of new code
  • for v2, to support v2 and v2.5, refactor the coordinator interface to a common set of behaviors that are implemented in v2 and v2.5 under the hood, due to the abi breakage
  • update the vrf job spec to specify which v2 version to use, valid inputs are "V2" or "V2_5"

still todo:

  • add native billing behavior in v2 listener
  • add integration tests for native billing behavior
  • add integration tests for subscription owner contract, migration, etc.
  • add sample contracts that pass costs to the end user
  • add unit tests for new contracts (forge?)

@github-actions
Copy link
Contributor

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

@jinhoonbang
Copy link
Contributor

Took a pass through the contract changes and it looks good to me. Will do another pass through tests soon. Some high level comments:

  1. Should we add some logic to incentivize LINK over native payment? It could be a configurable parameter that charges extra for native payments if both payment options are available.
  2. Should we consider globally unique subscription ID? might result in a more consistent and intuitive user experience across versions. That being said, I think the migration logic where new subscription ID is created on the new coordiantor should work too.
  3. s_requestPayments seems like a nice win! :)

@github-actions
Copy link
Contributor

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

@makramkd
Copy link
Contributor Author

Should we add some logic to incentivize LINK over native payment? It could be a configurable parameter that charges extra for native payments if both payment options are available.

As of now there are two different premiums - LINK and Native. You could reduce the LINK premium to incentivize.

Should we consider globally unique subscription ID? might result in a more consistent and intuitive user experience across versions. That being said, I think the migration logic where new subscription ID is created on the new coordiantor should work too.

Maybe, but I think it'd be a wash. It's easier to remember a shorter subscription ID than one that is a uint256. For the UI, we'd have to scope things by coordinator instead of just /<network>/<subid> but that doesn't seem like too big a lift. Open to discussing more.

s_requestPayments seems like a nice win! :)

That's still not final. Another option would be to callback again into the user contract with a small gas limit (say 25k) and let them do what they want with the value, but I feel like this is slightly cleaner.

In general I'd like to reduce scope creep; slight improvements are fine but the less major changes the better.

@jinhoonbang
Copy link
Contributor

Another option would be to callback again into the user contract with a small gas limit (say 25k) and let them do what they want with the value, but I feel like this is slightly cleaner.

Alternatively, payment amount can be part of rawFulfillRandomWords()

@makramkd
Copy link
Contributor Author

Alternatively, payment amount can be part of rawFulfillRandomWords()

Unfortunately it can't be because you have to execute the callback to figure out how much gas it uses exactly.

contracts/pnpm-lock.yaml Outdated Show resolved Hide resolved
jinhoonbang and others added 2 commits July 13, 2023 08:03
* integration tests for VRF V2 Plus. WIP

* support vrfv2 plus in bhs and bhf

* small fixes

* fix failing force fulfill test cases

* make changes to vrfv2 consumer contract to make batching tests pass

* fix import

* add ci profile

* fix v1 test

* prettier

* Update contracts/src/v0.8/vrf/testhelpers/VRFV2PlusExternalSubOwnerExample.sol

Co-authored-by: Makram <makramkd@users.noreply.github.com>

* Update contracts/src/v0.8/vrf/testhelpers/VRFV2PlusRevertingExample.sol

Co-authored-by: Makram <makramkd@users.noreply.github.com>

* Update contracts/src/v0.8/vrf/testhelpers/VRFV2PlusSingleConsumerExample.sol

Co-authored-by: Makram <makramkd@users.noreply.github.com>

* address comments

---------

Co-authored-by: Makram <makramkd@users.noreply.github.com>
@vreff vreff marked this pull request as ready for review July 13, 2023 16:36
Co-authored-by: Makram <makramkd@users.noreply.github.com>
@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

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

1 similar comment
@github-actions
Copy link
Contributor

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

@cl-sonarqube-production
Copy link

@vreff vreff added this pull request to the merge queue Jul 14, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 14, 2023
@jinhoonbang jinhoonbang added this pull request to the merge queue Jul 14, 2023
Merged via the queue into develop with commit 7b57ad4 Jul 14, 2023
@jinhoonbang jinhoonbang deleted the feature/eth-vrf branch July 14, 2023 18:50
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.

3 participants