Explicit bounds for premium config params in VRFCoordinatorV2_5 #12314

ibrajer · 2024-03-06T15:46:38Z

setConfig method will revert if premium percentages are above 155
added unit tests for coverage

github-actions · 2024-03-06T15:46:57Z

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

github-actions · 2024-03-06T15:51:59Z

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

leeyikjiun · 2024-03-07T02:38:02Z

contracts/src/v0.8/vrf/dev/VRFCoordinatorV2_5.sol

@@ -179,6 +180,9 @@ contract VRFCoordinatorV2_5 is VRF, SubscriptionAPI, IVRFCoordinatorV2Plus {
    if (fulfillmentFlatFeeNativePPM > 0 && fulfillmentFlatFeeLinkDiscountPPM >= fulfillmentFlatFeeNativePPM) {
      revert LinkDiscountTooHigh(fulfillmentFlatFeeLinkDiscountPPM, fulfillmentFlatFeeNativePPM);
    }
+    if (nativePremiumPercentage > 100 || linkPremiumPercentage > 100) {


You could define a MAX_PREMIUM_PERCENTAGE constant similar to the MAX_REQUEST_CONFIRMATIONS above. Don't worry about gas cost as constants are replaced during compile time. Defining such a constant will make the boundaries for the config clearer as well.

Makes sense!

Is there a situation where we would like to charge nativePremiumPercentage or linkPremiumPercentage as 150%. For example, when the value of LINK is very low against native.

According to this piece of code I figured that 100 should be the upper bound limit. As for your question about the described situation and what should we do in this case, probably @jinhoonbang is a better person to answer that.

Setting the premium percentage as 150% means we are charging 250% of the gas cost. I don't think the LINK / Native price matters here as we're still getting the same value just denominated in different tokens. I would think the ability to set the premium percentages differently for native or link acts another mechanism to incentive people to pay in one of them.

If we want to be very safe, we can set the limit to the max of 155

leeyikjiun · 2024-03-07T02:38:46Z

contracts/src/v0.8/vrf/dev/VRFCoordinatorV2_5.sol

@@ -179,6 +180,9 @@ contract VRFCoordinatorV2_5 is VRF, SubscriptionAPI, IVRFCoordinatorV2Plus {
    if (fulfillmentFlatFeeNativePPM > 0 && fulfillmentFlatFeeLinkDiscountPPM >= fulfillmentFlatFeeNativePPM) {
      revert LinkDiscountTooHigh(fulfillmentFlatFeeLinkDiscountPPM, fulfillmentFlatFeeNativePPM);
    }
+    if (nativePremiumPercentage > 100 || linkPremiumPercentage > 100) {
+      revert InvalidPremiumPercentage(nativePremiumPercentage, linkPremiumPercentage);


Not too sure if we need to follow the pattern of emitting the max value in the event.

Where did you see this pattern so I can take a look?

Here. https://github.com/smartcontractkit/chainlink/blob/develop/contracts/src/v0.8/vrf/dev/VRFCoordinatorV2_5.sol#L27-L29

Seems like the errors are emitting the allowable values as well.

I see - would that mean I have to create two categories of errors, one for native and another one for LINK? Or would just one be enough and you should be able to figure it out based on the have and want values returned? Consider I don't want to blow up the contract size just because I want to keep things clear.

I would say just the max value is enough? And since each premium percentage has the same max value, we could just keep it to a single error.

Should we send a string "native" or "link" in the event to identify which premium is over the bounds?

We can but I don't think it's needed? Like just stare at the two numbers and see which is over the limit. lol

I thought about it at the beginning, but I agree with @leeyikjiun here. Just by looking at the numbers, it should be clear which one is it. No need to increase the contract size with additional data.

kidambisrinivas · 2024-03-12T09:50:56Z

@ibrajer Please address the merge conflicts

* setConfig method will revert if premium percentages are above 100 * added unit tests for coverage

cl-sonarqube-production · 2024-03-12T13:07:45Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

* Explicit bounds for premium config params in VRFCoordinatorV2_5 * setConfig method will revert if premium percentages are above 100 * added unit tests for coverage * Generated Go wrappers and executed prettier tool * Added constants for percentage bounds and changed revert error * Removed min boundary * Added changeset * Changed upper premium percentage boundary to 155

ibrajer self-assigned this Mar 6, 2024

ibrajer temporarily deployed to sdlc March 6, 2024 15:46 — with GitHub Actions Inactive

ibrajer temporarily deployed to sdlc March 6, 2024 16:21 — with GitHub Actions Inactive

ibrajer marked this pull request as ready for review March 6, 2024 17:05

ibrajer requested review from a team as code owners March 6, 2024 17:05

leeyikjiun reviewed Mar 7, 2024

View reviewed changes

jinhoonbang approved these changes Mar 7, 2024

View reviewed changes

jinhoonbang previously approved these changes Mar 7, 2024

View reviewed changes

ibrajer dismissed jinhoonbang’s stale review via bc8a2a3 March 8, 2024 07:53

ibrajer temporarily deployed to sdlc March 8, 2024 07:53 — with GitHub Actions Inactive

ibrajer force-pushed the chore/VRF-912-invalid-premium-values branch from bc8a2a3 to 973ebae Compare March 8, 2024 07:56

ibrajer temporarily deployed to sdlc March 8, 2024 07:56 — with GitHub Actions Inactive

ibrajer temporarily deployed to sdlc March 8, 2024 08:50 — with GitHub Actions Inactive

ibrajer force-pushed the chore/VRF-912-invalid-premium-values branch from c54f031 to 656ad0e Compare March 11, 2024 12:32

ibrajer temporarily deployed to sdlc March 11, 2024 12:32 — with GitHub Actions Inactive

ibrajer force-pushed the chore/VRF-912-invalid-premium-values branch from 21e4dcf to 6548ae0 Compare March 12, 2024 10:15

ibrajer temporarily deployed to sdlc March 12, 2024 10:16 — with GitHub Actions Inactive

ibrajer added 6 commits March 12, 2024 13:48

Explicit bounds for premium config params in VRFCoordinatorV2_5

40b835d

* setConfig method will revert if premium percentages are above 100 * added unit tests for coverage

Generated Go wrappers and executed prettier tool

05b9d17

Added constants for percentage bounds and changed revert error

0bd67a6

Removed min boundary

fec3897

Added changeset

f93977b

Changed upper premium percentage boundary to 155

9315d5d

ibrajer force-pushed the chore/VRF-912-invalid-premium-values branch from 6548ae0 to 9315d5d Compare March 12, 2024 12:55

ibrajer temporarily deployed to sdlc March 12, 2024 12:55 — with GitHub Actions Inactive

kidambisrinivas approved these changes Mar 12, 2024

View reviewed changes

kidambisrinivas added this pull request to the merge queue Mar 12, 2024

Merged via the queue into develop with commit 15103b8 Mar 12, 2024
109 checks passed

kidambisrinivas deleted the chore/VRF-912-invalid-premium-values branch March 12, 2024 15:25

leeyikjiun mentioned this pull request Mar 13, 2024

Update VRFV2PlusWrapper billing parameters to mirror VRFCoordinatorV2_5 #12407

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Explicit bounds for premium config params in VRFCoordinatorV2_5 #12314

Explicit bounds for premium config params in VRFCoordinatorV2_5 #12314

ibrajer commented Mar 6, 2024 •

edited

Loading

github-actions bot commented Mar 6, 2024

github-actions bot commented Mar 6, 2024

leeyikjiun Mar 7, 2024

ibrajer Mar 7, 2024

kidambisrinivas Mar 8, 2024

ibrajer Mar 8, 2024

leeyikjiun Mar 8, 2024

leeyikjiun Mar 7, 2024

ibrajer Mar 7, 2024

leeyikjiun Mar 7, 2024 •

edited

Loading

ibrajer Mar 7, 2024

leeyikjiun Mar 7, 2024

kidambisrinivas Mar 12, 2024

leeyikjiun Mar 12, 2024

ibrajer Mar 12, 2024 •

edited

Loading

kidambisrinivas commented Mar 12, 2024

cl-sonarqube-production bot commented Mar 12, 2024

Explicit bounds for premium config params in VRFCoordinatorV2_5 #12314

Explicit bounds for premium config params in VRFCoordinatorV2_5 #12314

Conversation

ibrajer commented Mar 6, 2024 • edited Loading

github-actions bot commented Mar 6, 2024

github-actions bot commented Mar 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leeyikjiun Mar 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibrajer Mar 12, 2024 • edited Loading

Choose a reason for hiding this comment

kidambisrinivas commented Mar 12, 2024

cl-sonarqube-production bot commented Mar 12, 2024

Quality Gate passed

ibrajer commented Mar 6, 2024 •

edited

Loading

leeyikjiun Mar 7, 2024 •

edited

Loading

ibrajer Mar 12, 2024 •

edited

Loading