Separated signer from core components. #174
Conversation
sifnoc
requested review from
enricobottazzi and
alxkzmn
and removed request for
enricobottazzi
There was a problem hiding this comment.
Kudos for thinking about the nonce collision problem and finding an elegant solution for that.
I left some minor comment, but LGTM.
Relying on @alxkzmn review for checking that the testing is done correctly
| address_ownership_proofs: Vec<AddressOwnershipProof>, | ||
| signer: SummaSigner, | ||
| signer: &'a SummaSigner, | ||
| } |
There was a problem hiding this comment.
I understand (and appreciate) your choice of using a lifetime annotation for the SummaSigner reference. This is the only place in the code base we use that. Do you think there are other places in which this might be needed for managing references?
There was a problem hiding this comment.
The lifetime annotation is necessary because the signer instance is shared between both Round and AddressOwnership. I traced the flow of data like this: Round → Snapshot → MerkleSumTree → Node, Entry. There doesn't seem to be any need for data sharing along that chain.
The only improvement I identified is that there's no need to clone merkle_sum_tree when initializing circuits.
For instance, instead of
let mut circuit = MstInclusionCircuit::<LEVELS, N_ASSETS, N_BYTES>::init(merkle_sum_tree.clone(), 0);,
it could be
let mut circuit = MstInclusionCircuit::<LEVELS, N_ASSETS, N_BYTES>::init(&merkle_sum_tree, 0);.
There was a problem hiding this comment.
Good point! Gonna create an issue for that => #180
There was a problem hiding this comment.
Good point! Gonna create an issue for that
I got fixed one, let assign to me :)
| 1, | ||
| ) | ||
| .unwrap(); | ||
| let mut round = Round::<4, 2, 14>::new(&signer, entry_csv, asset_csv, params_path, 1).unwrap(); |
There was a problem hiding this comment.
can you create a variable named timestamp and assign 1 to that. Passing 1 directly to the new function might not be clear for the reader
| .unwrap(); | ||
| ); | ||
|
|
||
| let mut address_ownership_client = AddressOwnership::new(&signer, signature_csv_path).unwrap(); |
| @@ -136,18 +170,77 @@ mod test { | |||
| Ok(()) | |||
| } | |||
|
|
|||
| #[tokio::test] | |||
| async fn test_concurrent_proof_submissions() -> Result<(), Box<dyn Error>> { | |||
There was a problem hiding this comment.
Can you make much clear in the commments what is the goal of this test and how the results you are obtaining are proving that the goal was accomplished?
| // Using AddressInput::Address to directly provide the summa_contract's address. | ||
| // For deployed contracts, if the address is stored in a config file, | ||
| // you can alternatively use AddressInput::Path to specify the file's path. | ||
| let mut address_ownership_client = AddressOwnership::new( | ||
| let signer = SummaSigner::new( | ||
| "0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80", | ||
| anvil.chain_id(), |
There was a problem hiding this comment.
Is it possible to provide the chainId when instantiating the provider? It's better to have the rpc url and chain ID together in one place.
There was a problem hiding this comment.
Simply speaking, the provider url is chain-specific so there shouldn't be a discrepancy between the provider url and chain ID
There was a problem hiding this comment.
Do you think it's better to derive the chain_id directly from the URL, instead of assigning it explicitly?
There was a problem hiding this comment.
Actually I got into the ethers code and I see that there's an RPC method call inside that retrieves the chain ID from the node: provider.get_chainid(). I think with this in mind it's enough to create the provider from the URL and then get the chain ID.
AddressOwnershipandRoundnow require aSignerfor initialization. and they use same signer instance.Signerhas a mutex lock for preventing nonce collision.