Put the asset info in the CSV header #183 #192
Conversation
|
@enricobottazzi @sifnoc we'll also need to modify the CSVs used for benchmarks after this one is merged. |
There was a problem hiding this comment.
The PR looks fine. I just have some issue with the naming of this new field on MerkleSumTree as assets. While this is technically correct. it sounds confusing in the context of Proof of Solvency, as people associate assets with proof of assets, which is not the case here.
In our context Asset can be replaced by Currency, CryptoCurrency or Coin. Open to suggestion
backend/README.md
Outdated
| - `assets.csv`: Calculates the total balance of assets for the solvency proof. Only the CEX can generate this file. | ||
| - `entry_16.csv`: Used to build the Merkle sum tree, with each leaf element derived from sixteen entries in the CSV. | ||
| - `ptau/hermez-raw-11`: Contains parameters for constructing the zk circuits. | ||
| - `entry_16.csv`: contains the username and asset balance entries for each CEX user (necessary to build the commitment). Asset balance column names have the following format: `balance_<ASSET>_<CHAIN>`, where <ASSET> and <CHAIN> are the names of the assets and their corresponding blockchains. <CHAIN> values are the same as in the Address Ownership Proof step; |
There was a problem hiding this comment.
I find it confusing to name it asset balance entries when these are actually liabilities.
| @@ -1,6 +1,7 @@ | |||
| use crate::merkle_sum_tree::{Entry, MerkleProof, Node}; | |||
| use halo2_proofs::halo2curves::bn256::Fr as Fp; | |||
| use num_bigint::BigUint; | |||
|
|
|||
| use super::Asset; | |||
There was a problem hiding this comment.
Can you use use crate::merkle_sum_tree::Asset rather than super ? Just for coherence with the rest of the library
zk_prover/src/merkle_sum_tree/mst.rs
Outdated
| @@ -22,6 +22,7 @@ pub struct MerkleSumTree<const N_ASSETS: usize, const N_BYTES: usize> { | |||
| nodes: Vec<Vec<Node<N_ASSETS>>>, | |||
| depth: usize, | |||
| entries: Vec<Entry<N_ASSETS>>, | |||
| assets: Vec<Asset>, | |||
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
|
@enricobottazzi How about "cryptoasset"? |
enricobottazzi
merged commit 2bb8bff
into
summa-dev:v1-improvements-and-consolidation
Resolves #183.