Skip to content

Conversation

@kamilsa
Copy link
Contributor

@kamilsa kamilsa commented Sep 10, 2020

Signed-off-by: kamilsa [email protected]

Referenced issues

Fixes #520

Description of the Change

Fixes problem of wronlgy returned valur from ext_storage_read

Benefits

Conformance

Possible Drawbacks

None

Usage Examples or Tests

StorageExtensionTest

Signed-off-by: kamilsa <[email protected]>
@kamilsa kamilsa marked this pull request as draft September 10, 2020 09:28
Signed-off-by: kamilsa <[email protected]>
@kamilsa kamilsa marked this pull request as ready for review September 10, 2020 10:22
@kamilsa kamilsa requested review from Harrm and iceseer September 10, 2020 10:23
auto [value_ptr, value_size] = runtime::WasmResult(value_out);

auto key = memory_->loadN(key_ptr, key_size);
boost::optional<uint32_t> res{boost::none};
Copy link
Contributor

Choose a reason for hiding this comment

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

It initializes with none in ctor by def, i suppose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, but making it explicit improves readability I suppose

Comment on lines 440 to 441
auto encoded_opt_val_size =
kagome::scale::encode(boost::make_optional(value.length)).value();
Copy link
Contributor

Choose a reason for hiding this comment

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

EXPECT_OUTCOME_TRUE(encoded_opt_val_size, kagome::scale::encode(boost::make_optional(value.length)));

Signed-off-by: kamilsa <[email protected]>
@kamilsa kamilsa merged commit 72c668e into master Sep 11, 2020
@kamilsa kamilsa deleted the fix/ext_storage_read branch September 11, 2020 06:55
@FlorianFranzen
Copy link
Contributor

FlorianFranzen commented Sep 11, 2020

As far as I can tell from the code, the return value is neither to spec (data written), nor what substrate does (value size in storage) or claims to do (value size at offset), but the length of the buffer passed into the function, which the caller knows anyway.

After a bit of discussion, we settled for returning the amount of data in store at that offset (option 3) and Parity implemented a fix accordingly in paritytech/substrate#7084.

We will update the spec as well and track all progress, including progress on adding the ext_storage_read test to the kagome-adapter, in w3f/polkadot-spec#285.

@kamilsa
Copy link
Contributor Author

kamilsa commented Sep 11, 2020

Thanks! We will fix accordingly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix ext_storage_read_version_1

5 participants