[ethereum] - charge updateFee per number of updates #878

swimricky · 2023-06-12T16:07:54Z

Summary

change getUpdateFee to calculate the total fee based on the numUpdates for accumulator updates. Still uses same logic for batch price
lowers optimizer_runs to 2000 in order to get the bytesize down
- optimizer_runs = 5000 had deployment size of 25046
  
  diff between optimizer_runs = 2000 + this branch and main
getUpdateFeeWhMerkleX is high b/c the original implementation returned singleUpdateFee * updateData.length

Note

Will make separate PR for optimizing bytesize further and increasing optimizer_runs to get as close to original 5000 runs

vercel · 2023-06-12T16:07:59Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments

Name	Status	Preview	Comments	Updated (UTC)
example-oracle-amm	⬜️ Ignored (Inspect)			Jun 12, 2023 8:22pm
xc-admin-frontend	⬜️ Ignored (Inspect)			Jun 12, 2023 8:22pm

swimricky · 2023-06-12T16:08:36Z

target_chains/ethereum/contracts/contracts/pyth/Pyth.sol

-        uint requiredFee = getUpdateFee(updateData);
-        if (msg.value < requiredFee) revert PythErrors.InsufficientFee();
-
+        uint8 totalNumUpdates = 0;


@ali-bahjati is uint8 enough for holding the total number of updates we could get in one call?

yeah i'm not sure we can fit more gas-wise (the block gas limit). That's why on wire format it's u8. Do you think people will need more?

this would be for summing up all the individual numUpdates so depends on how big updateData could potentially be. I think for now 255 would be enough but we could possibly need more in the future?

oh ok ok. I might just change it to uint. Since word-size in evm is 32 bytes it shouldn't change much gas-wise.

ali-behjati · 2023-06-12T16:31:37Z

target_chains/ethereum/contracts/contracts/pyth/Pyth.sol

+        if (totalNumUpdates > 0) {
+            return totalNumUpdates * singleUpdateFeeInWei();
+        } else {
+            return updateData.length * singleUpdateFeeInWei();
+        }


You can make an else statement above and add totalNumUpdates by 1 if it's not accumulator. It will be cleaner.

also I think this should be consistent with the other places the fee is calculated. Use getRequiredFee (or whatever you refactor it to) everywhere so there's a single place with the calculation.

jayantk

great! I have a couple suggestions to improve this, but should be quick to do.

jayantk · 2023-06-12T16:48:58Z

target_chains/ethereum/contracts/contracts/pyth/Pyth.sol

                i++;
            }
        }
+        uint requiredFee = getRequiredFee(totalNumUpdates, updateData.length);


I think getRequiredFee is a confusing method because it embeds some conditional logic that isn't very easy for the reader to understand. Imo a better solution would be to have getFeeAccumulator and getFeeBatch, call those functions in the respective cases of the if, and have them set the requiredFee as a variable.

how about this instead?

if (totalNumUpdates == 0){ totalNumUpdates = updateData.length; } requiredFee = getTotalFee(totalNumUpdates);

the signatures between getFeeAccumulator & getFeeBatch are the same so for saving on bytesize maybe it would be best to not have 2 separate getFeeXXX methods?

jayantk · 2023-06-12T16:51:11Z

target_chains/ethereum/contracts/contracts/pyth/Pyth.sol

+        if (totalNumUpdates > 0) {
+            return totalNumUpdates * singleUpdateFeeInWei();
+        } else {
+            return updateData.length * singleUpdateFeeInWei();
+        }


also I think this should be consistent with the other places the fee is calculated. Use getRequiredFee (or whatever you refactor it to) everywhere so there's a single place with the calculation.

jayantk · 2023-06-12T17:00:22Z

target_chains/ethereum/contracts/contracts/pyth/Pyth.sol

-                        numUpdates
-                    ) = extractWormholeMerkleHeaderDigestAndNumUpdates(encoded);
+                        numUpdates,
+                        encoded


I don't like that this method returns encoded now -- it makes the method signature for this function extremely confusing.

BTW I find the overall handling of byte arrays in this code to be confusing. You're using two different techniques to slice out bytes (creating new arrays pointing to slices + offset indexes), so it's very hard for the reader to understand what an index into an array represents.

Imo the best way to handle this would be to have 1 byte array (the whole updateData) and 1 offset that increments as bytes are consumed.

(please don't address the comment about overall handling of byte arrays in this PR. You can do that separately if you'd like)

changing this to return encoded was a workaround for hitting the stack too deep error.

I spoke to @ali-bahjati regarding slicing out bytes vs just reading offsets and he said the implementation being used should be dependent on whether or not we're "throwing away" the data or need to possibly re-read it. I agree that there's areas where the implementation could be more consistent but there's also a few bytesize & gas cost considerations I'm also taking into account. I'll revisit this in a separate PR

jayantk · 2023-06-12T17:01:56Z

target_chains/ethereum/contracts/contracts/pyth/PythAccumulator.sol

        }
    }

+    function parseWormholeMerkleHeaderNumUpdates(


IMO it's better to reuse the method above instead of adding this. I don't think getUpdateFee needs to be efficient now, since it's only used by off-chain callers.

@jayantk I know that gas-aware protocols don't call this but I believe there are some people using getUpdateFee and it is also in our example contracts. (like this). So let's keep it.

ah yes good point about that on-chain usage.

swimricky · 2023-06-12T18:48:36Z

@ali-bahjati @jayantk
would not changing the batch price attestation updateFee to also be based on numAttestations possibly cause users to continue using batch prices due to cheaper fee?

target_chains/ethereum/contracts/contracts/pyth/Pyth.sol

target_chains/ethereum/contracts/forge-test/GasBenchmark.t.sol

…lean up unused code

swimricky · 2023-06-13T18:30:17Z

copying @jayantk's response to above question for records

i think that outcome is unlikely because (a) the fee is so tiny right now that it doesn't matter, and (b) we're going to turn off batch prices so it won't be possible to use them soon enough

feat(ethereum): charge update fee per numUpdates for accumulator updates

9032355

swimricky commented Jun 12, 2023

View reviewed changes

swimricky requested review from ali-behjati and jayantk and removed request for ali-behjati June 12, 2023 16:09

ali-behjati reviewed Jun 12, 2023

View reviewed changes

jayantk reviewed Jun 12, 2023

View reviewed changes

swimricky added 2 commits June 12, 2023 11:22

refactor(ethereum): refactor, add benchmarks for getUpdateFee

fac2fa0

refactor(ethereum): add back parseWormholeMerkleHeaderNumUpdates

023b1c1

swimricky requested review from ali-behjati and jayantk June 12, 2023 18:48

refactor: increment totalNumUdpates by 1 for batch prices

5845d63

jayantk approved these changes Jun 12, 2023

View reviewed changes

target_chains/ethereum/contracts/contracts/pyth/Pyth.sol Outdated Show resolved Hide resolved

target_chains/ethereum/contracts/forge-test/GasBenchmark.t.sol Outdated Show resolved Hide resolved

test(ethereum): add test for checking getUpdateFee for accumulator, c…

224f600

…lean up unused code

swimricky merged commit 19b77e2 into main Jun 13, 2023

swimricky deleted the update-fee-num-updates branch June 13, 2023 14:25

[ethereum] - charge updateFee per number of updates #878

[ethereum] - charge updateFee per number of updates #878

Uh oh!

Conversation

swimricky commented Jun 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Note

Uh oh!

vercel bot commented Jun 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jayantk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swimricky commented Jun 12, 2023

Uh oh!

Uh oh!

Uh oh!

swimricky commented Jun 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

swimricky commented Jun 12, 2023 •

edited

Loading

vercel bot commented Jun 12, 2023 •

edited

Loading