feat: starknet GMP #731
feat: starknet GMP #731
Conversation
packages/starknet-types/Cargo.toml
Outdated
| @@ -0,0 +1,28 @@ | |||
| [package] | |||
There was a problem hiding this comment.
are these types only used in ampd? or are they used in contracts as well? If they are only used in ampd, I think they should just live in a folder in ampd, close to where they are used. I see the packages folder as a place for things that are used across the entire repo.
There was a problem hiding this comment.
The StarknetMessage type is used in the multisig-prover. Should we keep it in our fork?
I can move the events and other types in ampd.
I'll use this thread to ask a question. We plan to migrate to using the cainome crate in our relayer. It has an abigen!() macro, which generates starknet types based on the ABI JSON file.
Do you prefer us to import that dependency and to maintain an ABI JSON file, or for us to leave the types in this folder? If we do use cainome - we'll delete all the types apart from StarknetMessage and use the generated ones. Else - we keep the types in this PR and only update if needed (rare to never).
There was a problem hiding this comment.
Please check this commit, in which I moved part of the modules to ampd and renamed starknet-types to starknet-message, keeping it in /packages, because it's used in the prover.
There was a problem hiding this comment.
yeah the prover code should live in your fork, and I'd prefer not to make changes to this repo that only really make sense for your fork. It makes things confusing. Eventually we will refactor ampd as well, so that starknet specific ampd code can also live in your fork as well.
I actually think I prefer the generated types. Can you try that and we see how it looks?
There was a problem hiding this comment.
It will require quite a refactor. Is it ok if we leave that for the optimisations phase of the project?
What has to be done to remove the files in the new /types folder:
- the cainome library will be included as a dependency.
- the ABI json file of the gateway contract will be added and needs to be kept up to date.
- we pass that file to the
abigen!()macro. - it generates a lot of code, including an RPC client for the contract to which the ABI json corresponds (the gateway contract). In
ampdwe need at most 5% of what it will generate. - we substitute the
ContractCall,SignersRotatedandByteArraytypes with the generated ones. - remove current implementations and fix all tests.
There was a problem hiding this comment.
Commit for removing starknet-message
…couldn't deserialise the key
| Some(gateway_event) => { | ||
| votes.push(verify_msg(&gateway_event, msg, &source_gateway_address)); | ||
| } | ||
|
|
||
| None => votes.push(Vote::NotFound), |
There was a problem hiding this comment.
why don't you just move this logic inside of the above map over unique_msgs? And then just generate the corresponding vote immediately after getting the event. There's no need for the events collection then. This seems a bit uneccessary to iterate twice like this, and to use mutable collections.
There was a problem hiding this comment.
It makes sense, yes. I mechanically changed the previous one.
It this better? (commit)
|
|
||
| let transaction_response = self | ||
| .rpc_client | ||
| .get_event_by_hash_signers_rotated(verifier_set.message_id.tx_hash.clone()) |
There was a problem hiding this comment.
correct me if I'm wrong, but it looks like the event index of the message id is being discarded? So one could put any value there, as long as the tx hash is correct. Is that right? If so, we should change that and also verify the event index
There was a problem hiding this comment.
We reworked it to also accept the message_id and get the event by it's index.
You can check this commit for the change.
…fier set (#58) * take into account index on checking for events in verify verifier set * remove useless tuple with felt and event; now its only event * fix functions name for verifier set and verify contract call * return None on verify verifier set, if something went wrong / remove result and some errors that applied to prev scenario * simplify logic of verify verifier set
Description
This PR consists of:
Todos
Steps to Test
You would need the Starknet relayer, in order to manually test, which is not open sourced or deployed yet.
Also we don't have integration or end-to-end tests yet.
Expected Behaviour
Handle GMP messages from other amplifier chains
Other Notes
The FieldElementAndEventIndex PR must be merged to main before this PR manages to compile successfully.