Skip to content

Fail when decoding from storage and not all bytes consumed#1897

Merged
ascjones merged 17 commits intomasterfrom
aj/decode-all
Oct 24, 2023
Merged

Fail when decoding from storage and not all bytes consumed#1897
ascjones merged 17 commits intomasterfrom
aj/decode-all

Conversation

@ascjones
Copy link
Copy Markdown
Collaborator

@ascjones ascjones commented Sep 7, 2023

Closes #1804.

⚠️ BREAKING CHANGE ⚠️

If a contract previously relied on successful decoding which does not consume all bytes, then recompiling with a version of ink! which includes this change will cause that contract to trap at runtime when attempting to decode.

See the new fn decode_all for Storable impls. It just copies the DecodeAll impl from parity-scale-codec which is very simple.

todo

@ascjones ascjones marked this pull request as ready for review October 23, 2023 13:07

/// Returns the decoded contract storage at the key if any.
pub fn get_storage(&mut self, key: &[u8], output: &mut &mut [u8]) -> Result {
pub fn get_storage(&mut self, key: &[u8]) -> core::result::Result<&[u8], Error> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can't you just import the full path?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

There was a type alias here type Result = core::result::Result<(), Error>;. However I've just removed that and replaced its usages with the explicit type.

if input.is_empty() {
Ok(res)
} else {
Err("Input buffer has still data left after decoding!".into())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perhaps would be useful to give the length of the remaining bytes left in a buffer for debugging purposes

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure it is worth bringing in all the string formatting machinery in for that...we need to be mindful of code size.

It's possible to debug this without this information by fetching the data at the given key and attempting to decode it into the respective type.

@paritytech-cicd-pr
Copy link
Copy Markdown

paritytech-cicd-pr commented Oct 23, 2023

🦑 📈 ink! Example Contracts ‒ Changes Report 📉 🦑

These are the results when building the integration-tests/* contracts from this branch with cargo-contract 4.0.0-alpha-acc02b1 and comparing them to ink! master:

Contract Upstream Size (kB) PR Size (kB) Diff (kB) Diff (%) Change
basic-contract-caller 2.998 2.998 0 0
basic-contract-caller/other-contract 1.331 1.338 0.007 0.52592 📈
call-builder-return-value 8.734 8.74 0.006 0.068697 📈
call-runtime 1.77 1.774 0.004 0.225989 📈
conditional-compilation 1.204 1.21 0.006 0.498339 📈
contract-terminate 1.087 1.092 0.005 0.459982 📈
contract-transfer 1.446 1.449 0.003 0.207469 📈
custom-allocator 7.39 7.429 0.039 0.52774 📈
dns 7.313 7.313 0 0
e2e-call-runtime 1.057 1.062 0.005 0.473037 📈
e2e-runtime-only-backend 1.633 1.64 0.007 0.428659 📈
erc1155 14.107 14.125 0.018 0.127596 📈
erc20 6.572 6.729 0.157 2.38892 📈
erc721 9.547 9.716 0.169 1.77019 📈
events 4.816 4.823 0.007 0.145349 📈
flipper 1.387 1.394 0.007 0.504686 📈
incrementer 1.217 1.223 0.006 0.493016 📈
lang-err-integration-tests/call-builder-delegate 2.321 2.327 0.006 0.258509 📈
lang-err-integration-tests/call-builder 4.875 4.88 0.005 0.102564 📈
lang-err-integration-tests/constructors-return-value 1.772 1.783 0.011 0.620767 📈
lang-err-integration-tests/contract-ref 4.363 4.363 0 0
lang-err-integration-tests/integration-flipper 1.565 1.572 0.007 0.447284 📈
mapping-integration-tests 2.975 3.216 0.241 8.10084 📈
mother 9.499 9.555 0.056 0.589536 📈
multi-contract-caller 5.977 5.993 0.016 0.267693 📈
multi-contract-caller/accumulator 1.09 1.096 0.006 0.550459 📈
multi-contract-caller/adder 1.672 1.672 0 0
multi-contract-caller/subber 1.693 1.693 0 0
multisig 21.406 21.628 0.222 1.03709 📈
payment-channel 5.544 5.559 0.015 0.270563 📈
sr25519-verification 0.865 0.87 0.005 0.578035 📈
static-buffer 1.411 1.418 0.007 0.496102 📈
trait-dyn-cross-contract-calls 2.491 2.491 0 0
trait-dyn-cross-contract-calls/contracts/incrementer 1.3 1.306 0.006 0.461538 📈
trait-erc20 6.956 7.113 0.157 2.25704 📈
trait-flipper 1.204 1.21 0.006 0.498339 📈
trait-incrementer 1.365 1.371 0.006 0.43956 📈
upgradeable-contracts/delegator 2.902 2.914 0.012 0.413508 📈
upgradeable-contracts/delegator/delegatee 1.368 1.374 0.006 0.438596 📈
upgradeable-contracts/set-code-hash 1.46 1.466 0.006 0.410959 📈
upgradeable-contracts/set-code-hash/updated-incrementer 1.439 1.445 0.006 0.416956 📈
wildcard-selector 2.619 2.625 0.006 0.229095 📈

Link to the run | Last update: Mon Oct 23 16:46:07 CEST 2023

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.

Consider using decode_all for get_contract_storage

5 participants