ink icon indicating copy to clipboard operation
ink copied to clipboard

Missing Limitation for Storage Size on Integration Testing for set_contract_storage()

Open ramirez7358 opened this issue 2 years ago • 1 comments

Summary

Closes #1960

  • [n] y/n | Does it introduce breaking changes?
  • [n] y/n | Is it dependant on the specific version of cargo-contract or pallet-contracts?

Description

Issue Description

The storage in the integration environment does not have the same limitation as in the e2e environment. In the e2e environment, the storage size is limited by 16380 bytes.

Issue Documentation & Test Case

https://github.com/CoinFabrik/on-ink-integration-tests/tree/main/test-cases/set-contract-storage

Current Status

Missing Limitation Implemented. Test Cases Passed. Pull Request Performed to Corresponding Repository.

Implementation Notes

The function set_contract_storage() sets the storage in a contract. There was a missing validation in integration tests that was present in e2e. This validation checked that the size of the storage set did not exceed 16380 bytes.

We added a validation to the function set_contract_storage() in integration tests that checks the size of the input value against the same limit set in e2e test: 16380 bytes. In case this limit is exceeded, a panic is raised with the message: "Value too large to be stored in contract storage, maximum size is {} bytes".

Updated Documentation

We updated the original documentation of this issue in our repository, adding the section Update on Correcting this Issue and informing of the correction.

There is no necessary update to the associated documentation: ink_env set_contract_storage.

Testing Guide

In the directory integration-tests/set_contract_storage of the target directory, we include in our pull request a test case showing that the observed implementation difference has been corrected. Note that this test is different from the original test case in our repository, which showed the difference.

In order to run the integration tests run:
cargo test

In order to run the e2e tests run:
cargo test -–features e2e-tests

These tests are run separately because we do not need to compare their results.

Checklist before requesting a review

  • [x] My code follows the style guidelines of this project
  • [x] I have added an entry to CHANGELOG.md
  • [x] I have commented my code, particularly in hard-to-understand areas
  • [x] I have added tests that prove my fix is effective or that my feature works
  • [x] Any dependent changes have been merged and published in downstream modules

ramirez7358 avatar Oct 30 '23 12:10 ramirez7358

@ramirez7358 Could you merge master into your PR and resolve the conflicts?

cmichi avatar Nov 14 '24 08:11 cmichi

Thanks for the work on this PR. It took a long while, but we now came to a decision that results in the closing of this PR. Some words on the reasoning and why it took so long:

The project moved out of Parity and with that came a period in which we didn't have funding and things were unclear. We resumed work a while ago to work on ink! v6; this is a migration away from WebAssembly and pallet-contracts to RISC-V and pallet-revive. We wrote down more context about this transition here.

While migrating to pallet-revive, we were uncertain how to handle the off-chain testing environment. We've now decided to remove it in favor of E2E tests against either a full node process or a sandboxed pallet-revive.

The reason for this decision is that the off-chain testing environment comes with a lot of maintenance costs and never fully reflected the on-chain behavior.

cmichi avatar Oct 14 '25 09:10 cmichi