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

Add log and warning about inserting empty values in trie. #6121

Closed
wants to merge 5 commits into from

Conversation

gui1117
Copy link
Contributor

@gui1117 gui1117 commented May 25, 2020

as a first step into fixing #5986

I updated doc and log so that it is clear that inserting an empty value is not supported. Test should fails with the debug_assert and in-production chain must log error when set an empty value in trie or child trie.

@gui1117 gui1117 added the A0-please_review Pull request needs code review. label May 25, 2020
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Why not also add the proposed change for decl_storage directly? I think if we have this, we will have covered 98% of all failures.

primitives/io/src/lib.rs Outdated Show resolved Hide resolved
primitives/io/src/lib.rs Outdated Show resolved Hide resolved
@gui1117
Copy link
Contributor Author

gui1117 commented May 25, 2020

Why not also add the proposed change for decl_storage directly? I think if we have this, we will have covered 98% of all failures.

You mean the const_assert(size_of($value_type) != 0) kind ?
Because some type have no size but encode to some values for instance the following type is of size 0:

#[derive(Encode)]
enum StorageVersion {
#[codec(index = "3")]
    _3
}

@gui1117
Copy link
Contributor Author

gui1117 commented May 25, 2020

though maybe I can try to make a const_assert that value_type must not be ()

@bkchr
Copy link
Member

bkchr commented May 25, 2020

I thought more about the following:

decl_storage! {
      Test: Vec<u8>,
      Test2: Option<u32>,
      Test3: Option<()>,
}

Will generate:

#[test]
fn test_Test_encode_is_not_empty() {
     assert!(!Test::get().encode().is_empty());
}

The Option<*> types are ignored for the tests, because we can not expect that the type inside the Option has a default value. But, we could generate a compile error for the Option<()> case directly.

I think that is the best we can do.

@@ -50,6 +50,9 @@ use proc_macro::TokenStream;
/// prefix. Instance prefix is "" for default instance and "Instance$n" for instance number $n.
/// Thus, instance 3 of module Example has a module prefix of `Instance3Example`
///
/// **Warning**: storage doesn't support inserting empty values, so the value must encode to
Copy link
Member

Choose a reason for hiding this comment

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

We should probably make this a sub-heading as well to make it more prominent, otherwise it may goes down in the overall text?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a section Warning at the end and kept this one.

@gui1117 gui1117 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 May 25, 2020
* add compile time test fr Option<()> and ()
* add runtime tests for value with defaults
* add warning section in decl_storage (end keep inline warning)
@gui1117
Copy link
Contributor Author

gui1117 commented May 25, 2020

done: tests generation for storage without option. And compile error for () and Option<()> value types.

@gui1117 gui1117 added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels May 25, 2020
@gui1117
Copy link
Contributor Author

gui1117 commented May 25, 2020

ok not sure this PR is wanted anymore, I can still finish it until we fix the support of empty values.

@bkchr
Copy link
Member

bkchr commented May 25, 2020

Yeah, not sure either.

@bkchr
Copy link
Member

bkchr commented May 25, 2020

Given the conversation in Substrate, I don't think that this is still needed.

@gui1117 gui1117 closed this May 26, 2020
@gui1117 gui1117 deleted the gui-warn-empty-values branch May 26, 2020 08:25
@gui1117 gui1117 removed the A0-please_review Pull request needs code review. label May 26, 2020
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.

3 participants