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

Update VRFV2PlusWrapper billing parameters to mirror VRFCoordinatorV2_5 #12407

Merged

Conversation

leeyikjiun
Copy link
Contributor

  • Split s_wrapperPremiumPercentage into s_wrapperNativePremiumPercentage and s_wrapperLinkPremiumPercentage
  • Changed s_fulfillmentFlatFeeLinkPPM into s_fulfillmentFlatFeeDiscountLinkPPM, a discount relative to s_fulfillmentFlatFeeNativePPM
  • Updated both s_fulfillmentFlatFeeNativePPM and s_fulfillmentFlatFeeDiscountLinkPPM to be denominated in native instead of LINK.
  • Updated _calculatedRequestPrice to be wrapper's gas cost + coordinator gas cost (inclusive of consumer) * premium + flat fee
  • Added premium percentage and flat fee checks from Explicit bounds for premium config params in VRFCoordinatorV2_5 #12314

Copy link
Contributor

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

@leeyikjiun leeyikjiun requested a review from a team as a code owner March 13, 2024 06:51
Copy link
Contributor

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


return feeWithFlatFee;
// (wei/gas) * gas
uint256 wrapperCostWei = _requestGasPrice * s_wrapperGasOverhead;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does the wrapper need its own L1 gas cost if it's running on L2?

Copy link
Contributor

Choose a reason for hiding this comment

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

my understanding is that L1 cost is per transaction. Since the coordinator calls wrapper in the same transaction, the L1 calldata cost added below for coordinator is sufficient

s_wrapperGasOverhead = _wrapperGasOverhead;
s_coordinatorGasOverhead = _coordinatorGasOverhead;
s_wrapperPremiumPercentage = _wrapperPremiumPercentage;
s_wrapperNativePremiumPercentage = _wrapperNativePremiumPercentage;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Didn't we discuss removing wrapper premium completely and just having a premium in coordinator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is kind of the coordinator's premium. This wrapperXPremiumPercentage has to mirror the value's set in the coordinator.

The wrapper's calculate price is roughly wrapper_gas + (coordinator_gas + consumer_gas) * premium + flat, simplified.

uint256 coordinatorCostWithPremiumAndFlatFeeWei = ((coordinatorCostWei * (s_wrapperNativePremiumPercentage + 100)) /
100) + (1e12 * uint256(s_fulfillmentFlatFeeNativePPM));

return wrapperCostWei + coordinatorCostWithPremiumAndFlatFeeWei;
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the previous code, the following qty is added to the return value for s_wrapperGasOverhead

(_requestGasPrice * s_wrapperGasOverhead * (s_wrapperPremiumPercentage + 100)) / 100

After this change, only the following qty is added to return value for s_wrapperGasOverhead

_requestGasPrice * s_wrapperGasOverhead

Is this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not quite sure if I'm following you. But yes, we removed the premium for the wrapper itself and will only keep the premium for the wrapper.

Previously it's something like ((gas price * (wrapper gas + coordinator gas + consumer gas)) + L1 gas cost) * premium multiplier + flat fee.

Now, after removing the premium and fees for wrapper, it becomes (gas price * wrapper gas) + ((gas price * (coordinator gas + consumer gas)) + L1 gas cost) * premium mulitplier + flat fee.

The only part I'm not sure is if we need the L1 gas cost for the wrapper part.

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

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

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

@cl-sonarqube-production
Copy link

@leeyikjiun leeyikjiun added this pull request to the merge queue Mar 20, 2024
Merged via the queue into develop with commit c6a7d02 Mar 20, 2024
115 of 116 checks passed
@leeyikjiun leeyikjiun deleted the VRF-921-VRF-V2-Plus-Wrapper-Remove-billing-parameters branch March 20, 2024 16:24
kidambisrinivas pushed a commit that referenced this pull request Mar 27, 2024
…_5 (#12407)

* Update VRFV2PlusWrapper billing parameters to mirror VRFCoordinatorV2_5

* Fix integration tests

* Fix Go scripts

* Fix calculate request price native using link premium percentage instead of native

* Update Go solidity wrappers

* vrfv2pluswrapper: optimize gas (#12428)

* Fix storage slot comment
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