Refactor backend following updated contracts #152

sifnoc · 2023-09-06T16:52:26Z

Update the backend API to reflect changes in the contracts.

Major changes include separating AddressOwnership from Snapshot, and introducing a new struct, Round, which is now included within Snapshot.

Modified gen_inclusion_verifier and gen_solvency_verifier scripts with using specific ptau file instead of using random.

zk_prover/examples/gen_solvency_verifier.rs

backend/src/apis/ownership.rs

enricobottazzi · 2023-09-08T10:28:50Z

backend/src/apis/ownership.rs

+        })
+    }
+
+    // Make sure the input `addresses` in `address_ownership_proof` are not duplicated with addresses already registered on the contract.


For what I can tell, this operation of making sure that the input 'addresses` ... is not being performed now so the comment is misleading. BTW I agree that we shouldn't do that.

Good point, that's why I also think that the address ownership should live inside the Round and Round should take care of it. (cc @sifnoc )

Why do you think so? One of the main ideas of this PR was to separate AddressOwnership from Round as these are separated phases

I'm not sure if they'll be performed separately in the real-world implementation. E.g., CEX can add more addresses in between the proving rounds but in each round it has to check if all the addresses have the ownership proofs submitted, so it makes sense just to submit new addresses in the beginning of the round. BTW I agree that it's good to separate address ownership into its own trait.

Updated comment at ce908f0

Both perspectives are valid. However, from my understanding, the AddressOwnership operation isn't intrinsically tied to Round. As such, it might not be fitting to merge AddressOwnership directly into Round at this juncture.
That being said, there could be value in creating a new struct that encompasses both functionalities.
Perhaps something like SummaOperator or SummaController?
For a CEX operator, a consolidated interface for both Round and AddressOwnership would undoubtedly be beneficial.

I now understand your perspective. You're suggesting a Round structure similar to this:

pub struct Round<const LEVELS: usize, const N_ASSETS: usize, const N_BYTES: usize> { timestamp: u64, snapshot: Snapshot<LEVELS, N_ASSETS, N_BYTES>, address_ownership: AddressOwnership, signer: SummaSigner, }

The following code segment, I see its alignment with your viewpoint on AddressOwnership:

summa-solvency/contracts/src/Summa.sol

Lines 107 to 110 in c4edbc6

require(

addressOwnershipProofs.length != 0,

"The CEX has not submitted any address ownership proofs"

);

However, regardless of the naming convention for Round, I believe there should be only one Signer.
If there are multiple signers, they might generate transactions with the same nonce.

I'm only talking about the encapsulation of AddressOwnership inside Round (Round holding an instance of AddressOwnership). As I imagine the future use of the system, CEX will not call any of Summa services from within CEX services, they will be called by the scheduler. If everything gets called with the same interval, it's all a "Round" of a proof of solvency.

My hypothesis is that Round and AddressOwnership are two very separate things and the Scheduler will only have to deal with Round. The soundness of this hypothesis depends on the frequency of address rotation performed by an Exchange. If they stick to the same address for long time, AddressOwnership is just called once and then just updated once in a while. On the contrary, if the switch addresses very frequently, it's likely that at every round a new proof of address ownership must be called.

I think we can stick to this now and challenge this hypothesis as soon as speak with customers. This will also inform the design of the Summa services.

However, regardless of the naming convention for Round, I believe there should be only one Signer.
If there are multiple signers, they might generate transactions with the same nonce.

Can elaborate on that @sifnoc ?

In the current implementation, both Round and AddressOwnership have an instance of Signer, each.
The default middleware in ethers-rs does not provide a shared nonce counter between signers.
As a result, both signers might query the node using get_transaction_count for the same Account.

This can lead to a high likelihood that both signers will generate transactions with the same nonce for submit_proof_of_solvency and submit_proof_of_address_ownership.

Consequently, one of these transactions may not be included in the block.

Yes, the same Signer should be injected everywhere where needed.

backend/src/apis/ownership.rs

backend/src/apis/round.rs

backend/src/apis/fetch.rs

backend/src/apis/round.rs

backend/src/apis/ownership.rs

alxkzmn · 2023-09-08T11:39:59Z

backend/src/tests.rs

-        let result = summa_signer
-            .submit_proof_of_address_ownership(owned_addresses)
+
+        let mut address_ownership_client = AddressOwnership::new(


I think that the address ownership should be a part of the Round (similarly to Snapshot). While it's possible to submit it at any time before the proof, it's still enough to submit it in the beginning of a round. Most likely it wouldn't be called manually in the production implementation of Summa like you're calling it here. There should be some entity that controls it and Round seems to be a good candidate for that.

#152 (comment) partially addresses your suggestion for now.
A potential alternative could be the Scheduler you introduced in the Service-Based Backend Architecture
Let's discuss this further in the document.

enricobottazzi

Just had a single comment on a variable name. LGTM!

enricobottazzi · 2023-09-12T07:21:38Z

backend/src/apis/csv_parser.rs

 #[derive(Debug, Deserialize)]
-struct Record {
+struct EntriesRecord {


Can we rename it to SignatureRecord as the other is named AssetRecord ?

Updated at 31c3c59

alxkzmn · 2023-09-12T08:44:20Z

backend/src/apis/csv_parser.rs

-    Ok((addresses, signatures))
+    let assets_array: [Asset; N_ASSETS] = assets_vec.try_into().map_err(|v: Vec<Asset>| {
+        format!(
+            "Number of assets in CSV does not match the number of assets in the contract! {:?}",


The error message is misleading, it should say something like "the number of assets doesn't match the expected"

Updated at 31c3c59

alxkzmn · 2023-09-12T09:00:20Z

backend/src/apis/ownership.rs

+        })
+    }
+
+    // Make sure the input `addresses` in `address_ownership_proof` are not duplicated with addresses already registered on the contract.


I'm not talking about merging (similarly to how Snapshot can still live in a separate file and not inside the same file with Round). I'm only talking about the encapsulation of AddressOwnership inside Round (Round holding an instance of AddressOwnership). As I imagine the future use of the system, CEX will not call any of Summa services from within CEX services, they will be called by the scheduler. If everything gets called with the same interval, it's all a "Round" of a proof of solvency.

sifnoc added 4 commits September 6, 2023 12:39

feat: create round struct

d3f73ba

fix: remove not used csv files and fix example

35bf5a2

feat: create address ownership struct

c1cfc2f

feat: create round test cases; still error with exist solvency verifier

23fbd50

sifnoc mentioned this pull request Sep 6, 2023

chore: update on gen_verifier scripts #151

Merged

sifnoc added 4 commits September 7, 2023 07:00

fix: re-arrange test cases for backend

d5f7d53

fix: updated verifier contracts that generated with specific patu file

f4ee087

chore: update README and remove unused

9d77d7d

fix: solved conflicts on merged master

c50eda9

sifnoc marked this pull request as ready for review September 7, 2023 08:48

sifnoc requested review from enricobottazzi and alxkzmn September 7, 2023 08:48

enricobottazzi requested changes Sep 8, 2023

View reviewed changes

This was referenced Sep 8, 2023

Add in backend readme description on how to run example. #134

Closed

Add whole flow example to backend examples #128

Closed

alxkzmn requested changes Sep 8, 2023

View reviewed changes

sifnoc added 4 commits September 11, 2023 12:57

fix: related address ownership

ce908f0

fix: related round and snapshot

3d1ab95

chore: removed fetching balances

9a93558

chore: udpate README

f5d04b1

sifnoc requested review from alxkzmn and enricobottazzi September 11, 2023 17:29

sifnoc mentioned this pull request Sep 11, 2023

Remove fetch API #145

Closed

enricobottazzi approved these changes Sep 12, 2023

View reviewed changes

This was linked to issues Sep 12, 2023

Remove fetch API #145

Closed

Refactor backend #144

Closed

Add in backend readme description on how to run example. #134

Closed

alxkzmn approved these changes Sep 12, 2023

View reviewed changes

fix: rename and comment

31c3c59

sifnoc mentioned this pull request Sep 12, 2023

Refactor signers #155

Closed

sifnoc merged commit bbf4363 into master Sep 12, 2023
4 checks passed

enricobottazzi deleted the refactor-backend branch November 21, 2023 14:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor backend following updated contracts #152

Refactor backend following updated contracts #152

sifnoc commented Sep 6, 2023 •

edited

Loading

enricobottazzi Sep 8, 2023

alxkzmn Sep 8, 2023

enricobottazzi Sep 8, 2023

alxkzmn Sep 8, 2023

sifnoc Sep 11, 2023

sifnoc Sep 12, 2023 •

edited

Loading

enricobottazzi Sep 12, 2023

enricobottazzi Sep 12, 2023

sifnoc Sep 12, 2023

alxkzmn Sep 12, 2023

alxkzmn Sep 8, 2023

sifnoc Sep 11, 2023

enricobottazzi left a comment

enricobottazzi Sep 12, 2023

sifnoc Sep 12, 2023

alxkzmn Sep 12, 2023

sifnoc Sep 12, 2023

alxkzmn Sep 12, 2023

	require(
	addressOwnershipProofs.length != 0,
	"The CEX has not submitted any address ownership proofs"
	);

Refactor backend following updated contracts #152

Refactor backend following updated contracts #152

Conversation

sifnoc commented Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sifnoc Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enricobottazzi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sifnoc commented Sep 6, 2023 •

edited

Loading

sifnoc Sep 12, 2023 •

edited

Loading