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

Use pallet-contracts-uapi #2038

Merged
merged 49 commits into from
Jan 24, 2024
Merged

Use pallet-contracts-uapi #2038

merged 49 commits into from
Jan 24, 2024

Conversation

pgherveou
Copy link
Contributor

@pgherveou pgherveou commented Jan 2, 2024

We now have a pallet-contracts-uapi crate (see issue paritytech/polkadot-sdk#2189) that expose all the host functions that a contract can import.

This PR remove the functions that were originally defined in https://github.com/paritytech/ink/blob/master/crates/env/src/engine/on_chain/ext.rs and use the one defined in this crate instead.

Some shared structs like Return and CallFlags are also now imported from this crate instead of being redefined in ink!

Note: Depends on the next crates.io release of pallet-contracts-uapi

Cargo.toml Outdated
Comment on lines 86 to 87
# TODO update once released
pallet-contracts-uapi = { path = "../polkadot-sdk/substrate/frame/contracts/uapi", default-features = false }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to wait upcoming release of this package before we can merge this

@pgherveou pgherveou marked this pull request as ready for review January 5, 2024 14:20
@codecov-commenter
Copy link

codecov-commenter commented Jan 5, 2024

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (db5600e) 53.47% compared to head (49d49dc) 53.73%.
Report is 22 commits behind head on master.

❗ Current head 49d49dc differs from pull request most recent head 987e869. Consider uploading reports for the commit 987e869 to get more accurate results

Files Patch % Lines
crates/env/src/engine/off_chain/impls.rs 44.44% 5 Missing ⚠️
crates/env/src/call/call_builder.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2038      +/-   ##
==========================================
+ Coverage   53.47%   53.73%   +0.25%     
==========================================
  Files         221      223       +2     
  Lines        6984     7040      +56     
  Branches     3082     3141      +59     
==========================================
+ Hits         3735     3783      +48     
- Misses       3249     3257       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

github-actions bot commented Jan 8, 2024

🦑 📈 ink! Example Contracts ‒ Changes Report 📉 🦑

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

Contract Upstream Size (kB) PR Size (kB) Diff (kB) Diff (%) Change
basic-contract-caller 3.207 3.281 0.074 2.30745 📈
basic-contract-caller/other-contract 1.581 1.574 -0.007 -0.442758 📉
call-builder-return-value 8.904 9.171 0.267 2.99865 📈
call-runtime 2.013 2.095 0.082 4.07352 📈
conditional-compilation 1.453 1.45 -0.003 -0.206469 📉
contract-storage 7.336 7.571 0.235 3.20338 📈
contract-terminate 1.336 1.336 0 0
contract-transfer 1.693 1.756 0.063 3.7212 📈
custom-allocator 7.652 7.652 0 0
dns 7.321 7.38 0.059 0.805901 📈
e2e-call-runtime 1.302 1.302 0 0
e2e-runtime-only-backend 1.879 1.874 -0.005 -0.266099 📉
erc1155 14.123 14.258 0.135 0.955888 📈
erc20 6.918 6.973 0.055 0.795027 📈
erc721 9.816 9.91 0.094 0.95762 📈
events 5.264 5.261 -0.003 -0.0569909 📉
flipper 1.637 1.63 -0.007 -0.427611 📉
incrementer 1.504 1.504 0 0
lang-err-integration-tests/call-builder-delegate 2.561 2.638 0.077 3.00664 📈
lang-err-integration-tests/call-builder 5.087 5.055 -0.032 -0.629054 📉
lang-err-integration-tests/constructors-return-value 1.987 1.98 -0.007 -0.35229 📉
lang-err-integration-tests/contract-ref 4.568 4.627 0.059 1.29159 📈
lang-err-integration-tests/integration-flipper 1.815 1.808 -0.007 -0.385675 📉
lazyvec-integration-test 4.553 4.62 0.067 1.47156 📈
mapping-integration-tests 7.919 8.014 0.095 1.19965 📈
mother 12.756 12.776 0.02 0.156789 📈
multi-contract-caller 6.155 5.936 -0.219 -3.55808 📉
multi-contract-caller/accumulator 1.378 1.378 0 0
multi-contract-caller/adder 1.908 1.968 0.06 3.14465 📈
multi-contract-caller/subber 1.928 1.988 0.06 3.11203 📈
multisig 21.621 21.676 0.055 0.254382 📈
payment-channel 5.653 5.689 0.036 0.63683 📈
sr25519-verification 1.148 1.148 0 0
static-buffer 1.649 1.649 0 0
trait-dyn-cross-contract-calls 2.706 2.792 0.086 3.17812 📈
trait-dyn-cross-contract-calls/contracts/incrementer 1.549 1.549 0 0
trait-erc20 7.294 7.371 0.077 1.05566 📈
trait-flipper 1.453 1.45 -0.003 -0.206469 📉
trait-incrementer 1.614 1.614 0 0
upgradeable-contracts/delegator 3.152 3.151 -0.001 -0.0317259 📉
upgradeable-contracts/delegator/delegatee 1.613 1.613 0 0
upgradeable-contracts/set-code-hash 1.747 1.802 0.055 3.14825 📈
upgradeable-contracts/set-code-hash/updated-incrementer 1.726 1.781 0.055 3.18656 📈
wildcard-selector 2.852 2.852 0 0

Link to the run | Last update: Tue Jan 23 18:10:18 CET 2024

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.

All in all looks pretty good.

It would still be interesting to know in a couple of cases (e.g. call-builder-return-value and contract-storage) why the contracts are 250 bytes or so bigger. I remember you looking into it previosly and overall the diffs don't seem as big now, and in many cases small reductions in size.

crates/env/src/engine/mod.rs Outdated Show resolved Hide resolved
crates/env/src/engine/mod.rs Outdated Show resolved Hide resolved
crates/env/src/lib.rs Show resolved Hide resolved
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

@pgherveou
Copy link
Contributor Author

mm pallet-contracts-uapi@5.0.0 is actually a buggy version, thus the tests are not passing here we will have to wait for the next release of the crate

@pgherveou
Copy link
Contributor Author

@ascjones, I published the package under ink-pallet-contracts-uapi so we can merge it now if that's ok for you

@ascjones ascjones merged commit 56a3025 into master Jan 24, 2024
23 checks passed
@ascjones ascjones deleted the pg/uapi branch January 24, 2024 17:36
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.

3 participants