Review updates

[Eitu33]

Hello,

This PR addresses the review suggestions you sent us, it contains all the changes we made following the review. However, there are some suggestions that we did not apply and we'll explain why here :

• For the serialize method of Id (renamed to to_bytes now) it’s impossible to return a slice of the object because the byte representation will be created inside the function. We could use a slice as a parameter and have the same behaviour as this crate : https://docs.rs/integer-encoding/latest/integer_encoding/trait.FixedInt.html#tymethod.encode_fixed but we don’t think it’s more suitable for our usage. Open to suggestions.

• For the errors we applied the changes you asked and we now use error types from DB/Felt conversion/Node decoding however we still have errors that hold a String for custom errors and for errors that need a context to be understandable.

• Your idea to use serde for the serialization of the changes is great but we think that it's too big of a machinery. The reasons are :

  • The conversion has only one purpose and so having the flexibility of the multiple deserialize format given by serde etc is useless
  • The conversion is only used in one place for an internal usage
  • This will not optimize the code and can make it more difficult to read

• For the TrieKey (which we have renamed to KeyOwned) we can’t use a slice with lifetime because we are saving a collection of these types in the structure of the trie (MerkleTree) but we are creating these objects when setting a new key in the trie or removing one. These objects don't live more than the tree because they are created when we manipulate it and so we can’t use a reference to store them in the trie.

• pub(super) on the type Path prevents us to make the comparison in the tests https://github.com/keep-starknet-strange/bonsai-trie/blob/4ae6caeb59429cee8efd51ebf821569b26caa91e/src/tests/proof.rs#L107

• The #[test] attribute already makes the function available only in test mode : https://doc.rust-lang.org/stable/reference/attributes/testing.html#the-test-attribute

• For the proposition of decoding directly in the key_value db we think it breaks the separation of concern and the simplicity of this structure KeyValueDB that is very straightforward for DB interactions. Moreover your T::decode() would lead to have a new trait implemented for all the values we want to decode which will lead to complexity and less usability of the KeyValueDB structure.

• “I see no reason why you cannot get rid of this and insert directly in self.storage_nodes” https://github.com/keep-starknet-strange/bonsai-trie/blob/87525315abac9611430c5f3c8db922f1d8e0e42f/src/trie/merkle_tree.rs#L464 It’s not possible because we already have a mutual borrow and so we would have two

[codecov]

Codecov Report

Attention: 39 lines in your changes are missing coverage. Please review.

Comparison is base (d2dd35d) 57.84% compared to head (fe355fe) 90.79%.
Report is 5 commits behind head on oss.

Files	Patch %	Lines
src/error.rs	0.00%	12 Missing ⚠️
src/trie/merkle_tree.rs	94.11%	8 Missing ⚠️
src/trie/path.rs	93.91%	7 Missing ⚠️
src/trie/trie_db.rs	77.27%	5 Missing ⚠️
src/databases/hashmap_db.rs	0.00%	3 Missing ⚠️
src/key_value_db.rs	83.33%	2 Missing ⚠️
src/databases/rocks_db.rs	87.50%	1 Missing ⚠️
src/lib.rs	95.83%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##              oss       #5       +/-   ##
===========================================
+ Coverage   57.84%   90.79%   +32.94%     
===========================================
  Files          11       17        +6     
  Lines         344     2205     +1861     
===========================================
+ Hits          199     2002     +1803     
- Misses        145      203       +58     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.