-
Notifications
You must be signed in to change notification settings - Fork 41
[FIXED#518]: storage extension: Fix return type of get #519
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: kamilsa <kamilsa16@gmail.com>
| // already scale-encoded | ||
| trie.put(key, value); | ||
| auto put_res = trie.put(key, value); | ||
| BOOST_ASSERT_MSG(put_res, "Trie put was failed"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better add logging, assert will be silent in release mode.
| // already scale-encoded | ||
| trie.put(key, value); | ||
| auto put_res = trie.put(key, value); | ||
| BOOST_ASSERT_MSG(put_res, "Trie put was failed"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| BOOST_ASSERT_MSG(put_res, "Trie put was failed"); | |
| BOOST_ASSERT_MSG(put_res, "Trie put failed"); |
|
|
||
| } else { | ||
| logger_->trace("ext_get_storage_into. Key hex: {} Value: empty", | ||
| logger_->trace("ext_storage_get_version_1( {} ) => not found", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not found is not the only error, so perhaps better to consider it.
| auto encoded = scale::encode(option).value(); | ||
|
|
||
| return memory_->storeBuffer(encoded); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rval?
return memory_->storeBuffer(scale::encode(option).value());
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also this code will be executed in the case !option. Is it wright?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean .value() will be failed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also this code will be executed in the case !option. Is it wright?
yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean
.value()will be failed.
scale::encode never fails)) we should change signature of this function later
Signed-off-by: kamilsa <kamilsa16@gmail.com>
Harrm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anxious about encode(...).value(), the rest is fine.
* storage extension: Fix return type of get * Fix storage extenstion test Signed-off-by: kamilsa <kamilsa16@gmail.com> * Fix review issues Signed-off-by: kamilsa <kamilsa16@gmail.com> Co-authored-by: Florian Franzen <Florian.Franzen@gmail.com>
Referenced issues
Fixes #514 . Fixed copy of #518
Description of the Change
Same as in #518
Benefits
Fixes issue with getting value from storage using ext_storage_get
Possible Drawbacks
None