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

Lazy: Account for key size in fallible storage ops #1962

Merged
merged 4 commits into from
Nov 6, 2023

Conversation

xermicus
Copy link
Contributor

Followup to #1910. The on-chain engine encodes the key and the value into the scoped buffer, so we need to account for both (same as Mapping).

Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
@xermicus xermicus requested a review from xgreenx October 30, 2023 14:31
@paritytech-cicd-pr
Copy link

paritytech-cicd-pr commented Oct 30, 2023

🦑 📈 ink! Example Contracts ‒ Changes Report 📉 🦑

⚠️ The ink! master is ahead of your branch, this might skew the comparison data below. ⚠️
These are the results when building the integration-tests/* contracts from this branch with cargo-contract 4.0.0-alpha-f7dd92d and comparing them to ink! master:

Contract Upstream Size (kB) PR Size (kB) Diff (kB) Diff (%) Change
basic-contract-caller 2.967 2.967 0 0
basic-contract-caller/other-contract 1.337 1.337 0 0
call-builder-return-value 8.735 8.735 0 0
call-runtime 1.769 1.769 0 0
conditional-compilation 1.209 1.209 0 0
contract-storage 7.171 7.171 0 0
contract-terminate 1.092 1.092 0 0
contract-transfer 1.444 1.444 0 0
custom-allocator 7.428 7.428 0 0
dns 7.249 7.249 0 0
e2e-call-runtime 1.058 1.058 0 0
e2e-runtime-only-backend 1.635 1.635 0 0
erc1155 13.968 13.962 -0.006 -0.0429553 📉
erc20 6.687 6.687 0 0
erc721 9.64 9.64 0 0
events 4.763 4.763 0 0
flipper 1.393 1.393 0 0
incrementer 1.221 1.221 0 0
lang-err-integration-tests/call-builder-delegate 2.317 2.317 0 0
lang-err-integration-tests/call-builder 4.847 4.847 0 0
lang-err-integration-tests/constructors-return-value 1.773 1.773 0 0
lang-err-integration-tests/contract-ref 4.328 4.328 0 0
lang-err-integration-tests/integration-flipper 1.571 1.571 0 0
mapping-integration-tests 7.685 7.685 0 0
mother 9.508 9.508 0 0
multi-contract-caller 5.924 5.924 0 0
multi-contract-caller/accumulator 1.095 1.095 0 0
multi-contract-caller/adder 1.669 1.669 0 0
multi-contract-caller/subber 1.689 1.689 0 0
multisig 21.471 21.471 0 0
payment-channel 5.501 5.501 0 0
sr25519-verification 0.865 0.865 0 0
static-buffer 1.405 1.405 0 0
trait-dyn-cross-contract-calls 2.466 2.466 0 0
trait-dyn-cross-contract-calls/contracts/incrementer 1.305 1.305 0 0
trait-erc20 7.063 7.063 0 0
trait-flipper 1.209 1.209 0 0
trait-incrementer 1.37 1.37 0 0
upgradeable-contracts/delegator 2.908 2.908 0 0
upgradeable-contracts/delegator/delegatee 1.369 1.369 0 0
upgradeable-contracts/set-code-hash 1.464 1.464 0 0
upgradeable-contracts/set-code-hash/updated-incrementer 1.443 1.443 0 0
wildcard-selector 2.622 2.622 0 0

Link to the run | Last update: Mon Nov 6 16:51:56 CET 2023

Copy link
Collaborator

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

LGTM

crates/storage/src/lazy/mod.rs Outdated Show resolved Hide resolved
crates/storage/src/lazy/mod.rs Outdated Show resolved Hide resolved
@xermicus
Copy link
Contributor Author

xermicus commented Nov 6, 2023

@mutantcornholio gitspiegel is failing, looks related to #1959?

@mutantcornholio
Copy link
Contributor

@xermicus yeah, I'm on it already. For some reason, doesn't reproduce in other repositories

mutantcornholio added a commit that referenced this pull request Nov 6, 2023
gitspiegel sync is failing on some already existing PRs, because the token from secrets isn't passed: #1962
This PR tests if new PRs are also affected
@mutantcornholio mutantcornholio mentioned this pull request Nov 6, 2023
7 tasks
@mutantcornholio
Copy link
Contributor

@xermicus seems that restarting a job doesn't update the secrets inside, could you try adding a commit, or rebasing please?

Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
@xermicus
Copy link
Contributor Author

xermicus commented Nov 6, 2023

@mutantcornholio just pushed again

@mutantcornholio
Copy link
Contributor

Turned out, that we've never tested this feature for PRs from forks...
A temporary fix is on the way: https://github.com/paritytech/gitspiegel/pull/179
And for the real fix: working on it

@xermicus xermicus merged commit 79d84e3 into use-ink:master Nov 6, 2023
23 of 24 checks passed
@xermicus
Copy link
Contributor Author

xermicus commented Nov 6, 2023

Alright, thanks @mutantcornholio 👍

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.

4 participants