use-ink / ink

Polkadot's ink! to write smart contracts.
https://use.ink
Apache License 2.0
1.35k stars 426 forks source link

Replace drink sandbox by internal ink_sandbox #2158

Closed pgherveou closed 7 months ago

pgherveou commented 7 months ago

Remove the drink dependency, and move the drink::Sandbox to it's own package.

codecov-commenter commented 7 months ago

Codecov Report

Attention: Patch coverage is 85.96491% with 16 lines in your changes are missing coverage. Please review.

Project coverage is 54.20%. Comparing base (72a671c) to head (8d8aaa6). Report is 2 commits behind head on master.

Files Patch % Lines
crates/e2e/sandbox/src/macros.rs 83.67% 8 Missing :warning:
crates/e2e/sandbox/src/api/contracts_api.rs 88.57% 4 Missing :warning:
crates/e2e/src/sandbox_client.rs 0.00% 2 Missing :warning:
crates/e2e/macro/src/config.rs 0.00% 1 Missing :warning:
crates/e2e/src/error.rs 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2158 +/- ## ========================================== + Coverage 53.64% 54.20% +0.56% ========================================== Files 225 231 +6 Lines 7095 7204 +109 Branches 3129 3153 +24 ========================================== + Hits 3806 3905 +99 - Misses 3289 3299 +10 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

github-actions[bot] commented 7 months ago

🦑 📈 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
call-builder-return-value 9.249 9.249 0 0 :heavy_minus_sign:
call-runtime 2.071 2.071 0 0 :heavy_minus_sign:
combined-extension 2.132 2.149 0.017 0.797373 :chart_with_upwards_trend:
conditional-compilation 1.502 1.502 0 0 :heavy_minus_sign:
contract-storage 7.58 7.58 0 0 :heavy_minus_sign:
contract-terminate 1.369 1.369 0 0 :heavy_minus_sign:
contract-transfer 1.731 1.731 0 0 :heavy_minus_sign:
cross-contract-calls 7.732 7.732 0 0 :heavy_minus_sign:
cross-contract-calls/other-contract 1.595 1.595 0 0 :heavy_minus_sign:
custom-allocator 7.787 7.787 0 0 :heavy_minus_sign:
custom-environment 2.158 2.158 0 0 :heavy_minus_sign:
dns 7.355 7.355 0 0 :heavy_minus_sign:
e2e-call-runtime 1.32 1.32 0 0 :heavy_minus_sign:
e2e-runtime-only-backend 1.901 1.901 0 0 :heavy_minus_sign:
erc1155 14.345 14.345 0 0 :heavy_minus_sign:
erc20 6.955 6.955 0 0 :heavy_minus_sign:
erc721 10.044 10.044 0 0 :heavy_minus_sign:
events 5.27 5.27 0 0 :heavy_minus_sign:
flipper 1.651 1.651 0 0 :heavy_minus_sign:
incrementer 1.516 1.516 0 0 :heavy_minus_sign:
lang-err-integration-tests/call-builder-delegate 2.65 2.65 0 0 :heavy_minus_sign:
lang-err-integration-tests/call-builder 5.571 5.571 0 0 :heavy_minus_sign:
lang-err-integration-tests/constructors-return-value 1.997 1.997 0 0 :heavy_minus_sign:
lang-err-integration-tests/contract-ref 5.062 5.062 0 0 :heavy_minus_sign:
lang-err-integration-tests/integration-flipper 1.827 1.827 0 0 :heavy_minus_sign:
lazyvec-integration-test 4.66 4.66 0 0 :heavy_minus_sign:
mapping-integration-tests 8.036 8.036 0 0 :heavy_minus_sign:
mother 12.753 12.753 0 0 :heavy_minus_sign:
multi-contract-caller 6.654 6.654 0 0 :heavy_minus_sign:
multi-contract-caller/accumulator 1.388 1.388 0 0 :heavy_minus_sign:
multi-contract-caller/adder 1.922 1.922 0 0 :heavy_minus_sign:
multi-contract-caller/subber 1.942 1.942 0 0 :heavy_minus_sign:
multisig 21.871 21.871 0 0 :heavy_minus_sign:
payment-channel 5.742 5.742 0 0 :heavy_minus_sign:
psp22-extension 7.083 7.083 0 0 :heavy_minus_sign:
rand-extension 2.977 2.977 0 0 :heavy_minus_sign:
sr25519-verification 1.154 1.154 0 0 :heavy_minus_sign:
static-buffer 2.578 2.578 0 0 :heavy_minus_sign:
trait-dyn-cross-contract-calls 2.899 2.899 0 0 :heavy_minus_sign:
trait-dyn-cross-contract-calls/contracts/incrementer 1.557 1.557 0 0 :heavy_minus_sign:
trait-erc20 7.331 7.331 0 0 :heavy_minus_sign:
trait-flipper 1.502 1.502 0 0 :heavy_minus_sign:
trait-incrementer 1.626 1.626 0 0 :heavy_minus_sign:
upgradeable-contracts/delegator 3.96 3.96 0 0 :heavy_minus_sign:
upgradeable-contracts/delegator/delegatee 1.641 1.641 0 0 :heavy_minus_sign:
upgradeable-contracts/delegator/delegatee2 1.641 1.641 0 0 :heavy_minus_sign:
upgradeable-contracts/set-code-hash-migration 1.755 1.755 0 0 :heavy_minus_sign:
upgradeable-contracts/set-code-hash-migration/migration 1.462 1.462 0 0 :heavy_minus_sign:
upgradeable-contracts/set-code-hash-migration/updated-incrementer 1.909 1.909 0 0 :heavy_minus_sign:
upgradeable-contracts/set-code-hash 1.755 1.755 0 0 :heavy_minus_sign:
upgradeable-contracts/set-code-hash/updated-incrementer 1.733 1.733 0 0 :heavy_minus_sign:
wildcard-selector 2.858 2.858 0 0 :heavy_minus_sign:

Link to the run | Last update: Tue Mar 19 16:52:03 CET 2024

deuszx commented 7 months ago

Won't this become a maintenance problem? Previously, when we wanted to extend capabilities of drink! we had to do it only for that one crate - drink! itself - now, there are two sandboxes - drink's and ink's. How do you envision the future of the tool if one of its biggest users is now ink! itself? drink is used not only as ink's native e2e test backend but standalone as well.

pgherveou commented 7 months ago

Won't this become a maintenance problem? Previously, when we wanted to extend capabilities of drink! we had to do it only for that one crate - drink! itself - now, there are two sandboxes - drink's and ink's. How do you envision the future of the tool if one of its biggest users is now ink! itself? drink is used not only as ink's native e2e test backend but standalone as well.

@deuszx to be clear, the goal is to move the shared part to ink!, not to duplicate the trait. The follow up will be to remove the code in drink! repo and use ink_sandbox as a dependency instead.

I agree that this is moving the maintenance burden, from one repo to the other, but because drink! already has (indirect) dependency on the ink! repo, it would make the overall maintenance much easier if we re-organize the dependency as suggested here.

What do you think?

pgherveou commented 7 months ago

drink! side of the migration https://github.com/inkdevhub/drink/pull/119

pgherveou commented 7 months ago

I have both side of the drink! sandbox migration ready to review. This is kind of annoying to rebase and maintain as a branch so ideally we can merge that quickly. cc @cmichi @ascjones @deuszx

deuszx commented 7 months ago

From my perspective looks good but let's consider separating release cycles of Sandbox and ink! as you mentioned in the comment.

pgherveou commented 7 months ago

LGTM

Thank you @pmikolajczyk41 @deuszx ❤️

I have excluded ink_sandbox from the release and added a note to the release, not sure if there is a better way to do it

See relevant commit here https://github.com/paritytech/ink/pull/2158/commits/11757409c9b05fdb1ab7988355585eb4d03d38ef