Skip to content
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

Make set_contract_storage fallible #1740

Closed
wants to merge 1 commit into from

Conversation

xermicus
Copy link
Contributor

@xermicus xermicus commented Apr 5, 2023

Related issue #1682

This is a very naive approach to the problem: If set_contract_storage can fail because the data does no longer fit into the encoding buffer, contract authors can account for that possibility. I don't think we need this for decoding because I don't see how something would fit in the buffer while encoding but no longer while decoding it.

A less invasive way would be to just add a try_set method to Lazy, but I thought this might be useful to have generally available. We also could introduce try_set_contract_storage into the API instead so we don't break the existing API. WDYT? Was there was an explicit reason set_contract_storage does not return a Result?

This is an early WIP. Todo:

  • Don't use Error::Unknown, might need a new one
  • Adapt integration tests
  • In the dispatch it just unwraps the result, however now that we have a Result we could as well do something about it other than panic
  • Implement tests
  • Probably many more

Signed-off-by: xermicus <[email protected]>
@xermicus xermicus added A-ink_env [ink_env] work item C-discussion An issue for discussion for a given topic. labels Apr 5, 2023
@xermicus
Copy link
Contributor Author

@ascjones @SkymanOne WDYT?

@SkymanOne
Copy link
Contributor

I don't think we need this for decoding because I don't see how something would fit in the buffer while encoding but no longer while decoding it.

with #1869 it is now possible for a contract to encode data using a larger buffer size, and then replace its code hash with a smaller buffer size and then fail. This can also pop up during cross-contract execution. So we need the same check for decoding.

@@ -20,6 +20,8 @@ use ink_primitives::Key;
/// [`scale::Codec`] are storable by default and transferable between contracts.
/// But not each storable type is transferable.
pub trait Storable: Sized {
fn size_hint(&self) -> usize;
Copy link
Contributor

Choose a reason for hiding this comment

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

size_estimate perhaps is a better name?

Copy link
Contributor

@SkymanOne SkymanOne left a comment

Choose a reason for hiding this comment

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

Look like a good start I would personally add try_set_contract_storage() to avoid breaking changes and be as close to Rust interface (e.g. from() and try_from()) as possible.

I would also like to hear more how this work would lead to the implementation of StorageVec as you mentioned it as a related issue.

@xermicus
Copy link
Contributor Author

Thanks @SkymanOne for your comments, good points. I'll pick it up again.

@xermicus
Copy link
Contributor Author

I would also like to hear more how this work would lead to the implementation of StorageVec as you mentioned it as a related issue.

Ah, so the problem this PR solves is more that currently there is no way to probe whether setting the storage will fail because of the encoded data size,

@xermicus
Copy link
Contributor Author

Closing in favor of #1910

@xermicus xermicus closed this Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ink_env [ink_env] work item C-discussion An issue for discussion for a given topic.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants