-
Notifications
You must be signed in to change notification settings - Fork 308
[eth] Add Pyth Accumulator #776
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
c322ce5 to
71f095d
Compare
jayantk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
message looks good except this one minor thing
jayantk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in general, this looks great. Nice job on very thorough testing. I left a couple minor comments
target_chains/ethereum/contracts/contracts/libraries/MerkleTree.sol
Outdated
Show resolved
Hide resolved
| if (i < messages.length) { | ||
| tree[(1 << depth) + i] = leafHash(messages[i]); | ||
| } else { | ||
| tree[(1 << depth) + i] = leafHash(""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will have to update this for the empty node prefix as well
| function updatePriceFeeds( | ||
| bytes[] calldata updateData | ||
| ) public payable override { | ||
| // TODO: Is this fee model still good for accumulator? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm yeah maybe we should charge on a per-feed basis, though doing that gas efficiently may be tricky. we may be able to do something a little too clever by using the number of bytes in an update as a proxy....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should do it per price and i like the idea of using number of bytes. What's wrong if i say len(update) * baseFee? 🤔
target_chains/ethereum/contracts/contracts/pyth/PythAccumulator.sol
Outdated
Show resolved
Hide resolved
target_chains/ethereum/contracts/contracts/pyth/PythAccumulator.sol
Outdated
Show resolved
Hide resolved
target_chains/ethereum/contracts/contracts/pyth/PythAccumulator.sol
Outdated
Show resolved
Hide resolved
target_chains/ethereum/contracts/contracts/pyth/PythAccumulator.sol
Outdated
Show resolved
Hide resolved
target_chains/ethereum/contracts/forge-test/Pyth.WormholeMerkleAccumulator.t.sol
Show resolved
Hide resolved
|
Merging this PR. There are two outstanding TODOs for the future: (1) update fee (2) whether to emit an accumulator update or not. |
This PR adds the support WormholeMerkle accumulator message to the ethereum contract while still supporting the old message format. The code is not optimized yet and with more optimizations we can achieve a better gas usage. Currently based on the gas benchmark below it has a 18% improvement with a single price feed. Although the cost of updating 5 feeds in the same batch is higher than the current approach but in reality the chances that all 5 feeds be in the same batch is very low.
In the future when the migration to the Merkle Accumulator is over we can break the backward compatibility and remove the old code.
This is the result of the benchmark:
Note: This change set adds an overhead of 1k gas to the batch attestations.
Note 2: Gas benchmark here does not include the initial input size cost and that will be lower too. So I can say the gas saving would be slightly more than 20%.