Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Conversation

@sorpaas
Copy link
Contributor

@sorpaas sorpaas commented Aug 16, 2019

This refactoring adds ChildTrie trait. The aim is to simplify child trie types addition.

With this PR, to add a new child trie type, one only needs to:

  • Create a new file under core/trie/src/child_tries, adds a new unit struct, and implement ChildTrie.
  • Modify the with_child_trie macro, add a new if condition defining the new prefix.

@sorpaas sorpaas added the I7-refactor Code needs refactoring. label Aug 16, 2019
@sorpaas sorpaas requested a review from cheme August 16, 2019 13:19
The child trie fetch should start with `:default:` prefix.
@gui1117 gui1117 added I5-tests Tests need fixing, improving or augmenting. and removed I5-tests Tests need fixing, improving or augmenting. labels Aug 21, 2019
@gui1117
Copy link
Contributor

gui1117 commented Aug 21, 2019

tests fail

@ddorgan ddorgan closed this Aug 22, 2019
@ddorgan ddorgan reopened this Aug 22, 2019
Copy link
Contributor

@cheme cheme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, maybe as small doc around the macro to make it goal explicit could be good (resolving child trie implementation from storage_key).

use hash_db::{HashDB, HashDBRef, PlainDB, PlainDBRef};

/// Definition for a child trie.
pub trait ChildTrie {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we may rename ChildTrie with something a bit more generic, I would say ChildState but that is not really related to the following methods either, so here to me KeyValueState is what makes the more sense.
Similarily we can remove the 'child' info here: there is nothing related to the child nature.
Still If we add method or associated const such as

  /// ChildTrie storage key defines its type. eg: the default child trie is using
  /// well_known_keys:: CHILD_STORAGE_KEY_PREFIX followed by ':' then
  // 'default', default being this associated const.
  const TYPE_PREFIX: &'static[u8] =  b"default:";

or

 get_type_prefix() -> &'static[u8];

/// Definition for a child trie.
pub trait ChildTrie {
/// Default root of the child trie.
fn default_root<L: TrieConfiguration>(&self) -> Vec<u8>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for all those function I would consider removing type parameter L, and pushing it into the struct

struct DefaultTrie<'a, L, DB, Query>{
  root: &'a[u8],
  _ph: PhantomData<(L, DB, Query)>
}

instead of

struct DefaultChildTrie;

Actually this comes with some constraint (probably associated type for prover and and memorydb), so it may not be worth it at this point.

/// Various re-exports from the `hash-db` crate.
pub use hash_db::{HashDB as HashDBT, EMPTY_PREFIX};

macro_rules! with_child_trie {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that it will be good to have a constant reference instead of b"child_storage:default:" and if we put a function/constant in ChildTrie trait to define its type prefix, it should also be use.

@gavofyork gavofyork added A0-please_review Pull request needs code review. and removed I7-refactor Code needs refactoring. labels Sep 21, 2019
use trie_db::{TrieConfiguration, DBValue, Query, Trie, TrieMut};
use hash_db::{HashDB, HashDBRef, PlainDB, PlainDBRef};

/// Default child trie.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Useless docs.

use trie_db::{TrieConfiguration, DBValue, Query};
use hash_db::{HashDB, HashDBRef, PlainDB, PlainDBRef};

/// Definition for a child trie.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Useless docs.


/// Definition for a child trie.
pub trait ChildTrie {
/// Default root of the child trie.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Useless docs

@sorpaas sorpaas requested a review from tomusdrw as a code owner September 21, 2019 08:50
@sorpaas sorpaas added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Sep 30, 2019
@gnunicorn
Copy link
Contributor

is still being worked on?

@gnunicorn gnunicorn removed the A3-in_progress Pull request is in progress. No review needed at this stage. label Feb 18, 2020
@sorpaas
Copy link
Contributor Author

sorpaas commented Feb 27, 2020

Sorry. Let me close this for now.

@sorpaas sorpaas closed this Feb 27, 2020
@sorpaas sorpaas deleted the sp-child-trie-refactor branch February 27, 2020 22:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants