feat: wormhole receiver contract for stylus #2774
Conversation
…verify with hermes VAA
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| @@ -0,0 +1,9 @@ | |||
| [package] | |||
| name = "pyth-contract" | |||
There was a problem hiding this comment.
Can we use a more specific name here like "pyth-stylus-receiver"?
There was a problem hiding this comment.
on second thought, we can also just remove this crate until we're ready to implement it to keep this PR atomic
target_chains/stylus/Cargo.toml
Outdated
| resolver = "2" | ||
|
|
||
| [workspace.package] | ||
| edition = "2021" |
There was a problem hiding this comment.
If we're specifying channel = "1.87.0" in rust-toolchain.toml, we can bump edition to "2024".
There was a problem hiding this comment.
move this to the workspace level?
There was a problem hiding this comment.
We need this file here for stylus to be able to build
target_chains/stylus/src/main.rs
Outdated
There was a problem hiding this comment.
Let's remove the placeholder files and dirs until we need them (src/, sdk/, etc)
There was a problem hiding this comment.
are these licenses necessary to include here? we have a blanket LICENSE (apache 2.0) at the repo root
| let hash = Keccak256::digest(&pubkey_uncompressed[1..]); // Skip 0x04 | ||
| let address_bytes: [u8; 20] = hash[12..].try_into().unwrap(); // Last 20 bytes |
There was a problem hiding this comment.
why skip and slice these specific bytes?
| ] | ||
| } | ||
|
|
||
| fn current_guardians() -> Vec<Address> { |
There was a problem hiding this comment.
these will change over time, and shouldn't be functionally different than the other guardian_set fixtures you've got here -- would recommend renaming it mock_guardian_set13.
also in general i'd recommend against calling certain tests "real" (are those tests considered better/more realistic than the non-real ones?) all fixtures should ideally be representative of real world inputs.
| assert_eq!(result.signatures.len(), 13) | ||
| } | ||
|
|
||
| #[motsu::test] |
There was a problem hiding this comment.
what does motsu give us? have just seen cargo test used in our other projects
There was a problem hiding this comment.
motsu is a package that mocks up the requirements of the stylus environment while doing unit tests
| #[motsu::test] | ||
| fn test_verify_vm_invalid_guardian_index() { | ||
| let contract = deploy_with_test_guardian(); | ||
| let signatures = vec![ | ||
| create_guardian_signature(5), | ||
| ]; | ||
| let vm = create_test_vm(0, signatures); | ||
|
|
||
| let result = contract.verify_vm(&vm); | ||
| assert!(matches!(result, Err(WormholeError::InvalidGuardianIndex))); | ||
| } |
There was a problem hiding this comment.
can we also test this with multiple guardian sets loaded into storage?
| } | ||
|
|
||
| #[motsu::test] | ||
| fn test_multiple_guardian_sets() { |
There was a problem hiding this comment.
great test coverage overall!!
There was a problem hiding this comment.
I will need it for deployment, I am still figuring out exactly the best way to handle it though.
| // #[cfg(all(not(feature = "std"), not(test)))] | ||
| // #[panic_handler] | ||
| // fn panic(_info: &core::panic::PanicInfo) -> ! { | ||
| // loop {} | ||
| // } |
There was a problem hiding this comment.
what's the context behind this :? shall we remove it?
There was a problem hiding this comment.
as an overal comment i recommend breaking this down to multiple modules.
There was a problem hiding this comment.
Have separated the tests out into a different module on a different PR, will also think about the best way to branch these out more
| VerfiyVMError, | ||
| } | ||
|
|
||
| impl From<WormholeError> for Vec<u8> { |
There was a problem hiding this comment.
maybe vec is what stylus accepts
| pub hash: FixedBytes<32>, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone)] |
There was a problem hiding this comment.
not sure whether it's supported in stylus or not, but thiserror can help to give you Display for the error.
| current_guardian_set_index: StorageUint<256, 4>, | ||
| chain_id: StorageUint<256, 4>, | ||
| governance_chain_id: StorageUint<256, 4>, |
There was a problem hiding this comment.
damn. this struct stores u64[4] inside instead of u8[256] to probably be more optimized. the 4 is a constant and is checked at compile time to make sure it's the right number for 256 bits.
| } | ||
| } | ||
|
|
||
| pub fn parse_and_verify_vm(&self, encoded_vm: Vec<u8>) -> Result<Vec<u8>, Vec<u8>> { |
There was a problem hiding this comment.
this is the name in EVM implementations and I recommend sticking to the same name. ideally we want anyone from EVM to be able to interact with this contract just as it was the Pyth/Wormhole EVM contract.
| if !self.initialized.get() { | ||
| return Err(WormholeError::NotInitialized.into()); | ||
| } |
There was a problem hiding this comment.
it's kind of strange that we need to check it everywhere
There was a problem hiding this comment.
Is there a better way to do this? My other thought was to initialize the contract if we run into it when it's not initialized.
| let timestamp = u32::from_be_bytes([ | ||
| encoded_vm[cursor], | ||
| encoded_vm[cursor + 1], | ||
| encoded_vm[cursor + 2], | ||
| encoded_vm[cursor + 3], | ||
| ]); |
There was a problem hiding this comment.
can't this be like let timestamp = u32::from_be_bytes(encoded_vm.get(cursor..cursor+4)).ok_or(WormholeError::InvalidVMFormat)
There was a problem hiding this comment.
Yeah, didn't know about this until Tejas mentioned it to me, should make things cleaner.
| cursor += 1; | ||
|
|
||
| let mut signature_bytes = [0u8; 65]; | ||
| signature_bytes.copy_from_slice(&encoded_vm[cursor..cursor + 65]); |
There was a problem hiding this comment.
generally instead of direct memory access you can use .get() that returns an Option and you can convert it to error and return if it's empty. this will be safer than manually checking size above and having a direct access.
Summary
Implementing a Wormhole receiver contract for the stylus framework. The receiver currently handles parsing + verifying VAAs and will also be able to handle guardian set management.
Rationale
A Wormhole receiver is necessary for ultimately implementing a Pyth receiver in Stylus.
How has this been tested?
I'm building a test suite for the stylus contract through normal cargo tests. At the moment those tests are focused on VAA verification, more tests will follow for guardian set management soon. I'll be expanding those tests and also running more complete stylus tests once I have everything loaded.