Conversation
Code ReviewThe PR adds a new One concern (non-blocking):
No critical issues found. The logic for unanimous support checking is correct ( ✅ Approved |
Migrations always happen in running state. We always use this pattern in the From implementation for previous states. |
netrome
left a comment
There was a problem hiding this comment.
Mostly looks good to me but I find the votes_per_chain name misleading which made me confused when I initially read this.
There was a problem hiding this comment.
There is quite a bit of code duplication in this file.
It would be nice to introduce helper methods for registering and chain configurations and fetching supported chains, such that we reduce boilerplate and make it easier to write or extend tests.
|
|
||
| // Then: only Bitcoin is in the supported set (Starknet is not unanimous) | ||
| let supported: Vec<String> = contract | ||
| .view("get_supported_foreign_chains") |
There was a problem hiding this comment.
Lets replace this string with GET_SUPPORTED_FOREIGN_CHAINS from the contract interface crate.
| fn test_migration_derives_supported_foreign_chains_from_policy() { | ||
| let policy = make_policy(vec![ | ||
| dtos::ForeignChain::Bitcoin, | ||
| dtos::ForeignChain::Ethereum, | ||
| ]); | ||
|
|
||
| let supported: dtos::SupportedForeignChains = policy | ||
| .chains | ||
| .keys() | ||
| .copied() | ||
| .collect::<BTreeSet<_>>() | ||
| .into(); | ||
|
|
||
| assert!(supported.contains(&dtos::ForeignChain::Bitcoin)); | ||
| assert!(supported.contains(&dtos::ForeignChain::Ethereum)); | ||
| assert_eq!(supported.len(), 2); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_migration_derives_empty_supported_foreign_chains_from_empty_policy() { | ||
| let policy = dtos::ForeignChainPolicy { | ||
| chains: BTreeMap::new(), | ||
| }; | ||
|
|
||
| let supported: dtos::SupportedForeignChains = policy | ||
| .chains | ||
| .keys() | ||
| .cloned() | ||
| .collect::<BTreeSet<_>>() | ||
| .into(); | ||
|
|
||
| assert!(supported.is_empty()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_migration_supported_foreign_chains_preserves_all_chain_keys() { | ||
| let all_chains = vec![ | ||
| dtos::ForeignChain::Solana, | ||
| dtos::ForeignChain::Bitcoin, | ||
| dtos::ForeignChain::Ethereum, | ||
| dtos::ForeignChain::Base, | ||
| dtos::ForeignChain::Bnb, | ||
| dtos::ForeignChain::Arbitrum, | ||
| ]; | ||
| let policy = make_policy(all_chains.clone()); | ||
|
|
||
| let supported: dtos::SupportedForeignChains = policy | ||
| .chains | ||
| .keys() | ||
| .cloned() | ||
| .collect::<BTreeSet<_>>() | ||
| .into(); | ||
|
|
||
| assert_eq!(supported.len(), all_chains.len()); | ||
| for chain in &all_chains { | ||
| assert!(supported.contains(chain)); |
There was a problem hiding this comment.
I must be missing something. We are not testing actual migration logic here, but verifying properties of the output of make_policy, which is a function defined in this test module.
If we want to test the actual migration logic, we need to construct an instance of MpcContract and test the migration method above, or break the relevant logic out to a function and test that in isolation.
| self.foreign_chain_policy_votes.to_dto() | ||
| } | ||
|
|
||
| pub fn get_supported_foreign_chains(&self) -> dtos::SupportedForeignChains { |
There was a problem hiding this comment.
Do we have benchmarks for the gas costs of this method?
Did we consider alternative data structures for foreign_chain_configuration_by_node?
It's currently IterableMap<dtos::AccountId, dtos::ForeignChainConfiguration>, which is a nested map:
Participant <-> { ForeignChain <-> Set<RpcProvider>> }, which essentially is: AccountId <-> {Enum <-> Set<String> }
It seems to me this is optimizing for insertion, but not for lookup.
Right now, we only use it to check whether a specific foreign chain is supported by all participants, which would be more suitable for IterableMap<ForeginChain, BTreeSet<AccountId>>.
There was a problem hiding this comment.
It's a read method. We don't use gas for reads
Edit: It's used in the verify foreign transaction function
kevindeforth
left a comment
There was a problem hiding this comment.
some comments, but nothing blocking.
closes #2783
This PR allows nodes to submit their RPC configuration on chain, and exposes a new API to get supported chains which doesn't require all nodes to use same RPC urls. Instead a chain is considered "supported" as long as all nodes have a configuration of it, regardless of their RPC urls.
The previous API to submit "policies" will be deprecated in a follow up PR.
For all the changes that will be done see #2779