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

Replace drink sandbox by internal ink_sandbox #2158

Merged
merged 27 commits into from
Mar 19, 2024
Merged

Replace drink sandbox by internal ink_sandbox #2158

merged 27 commits into from
Mar 19, 2024

Conversation

pgherveou
Copy link
Contributor

@pgherveou pgherveou commented Mar 14, 2024

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

@codecov-commenter
Copy link

codecov-commenter commented Mar 14, 2024

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 ⚠️
crates/e2e/sandbox/src/api/contracts_api.rs 88.57% 4 Missing ⚠️
crates/e2e/src/sandbox_client.rs 0.00% 2 Missing ⚠️
crates/e2e/macro/src/config.rs 0.00% 1 Missing ⚠️
crates/e2e/src/error.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            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     

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

@pgherveou pgherveou requested a review from a team as a code owner March 14, 2024 15:51
.github/workflows/ci.yml Outdated Show resolved Hide resolved
Copy link

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

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

crates/env/Cargo.toml Outdated Show resolved Hide resolved
@pgherveou pgherveou changed the base branch from master to pg/remove_verbose_logs March 15, 2024 08:19
@pgherveou pgherveou changed the base branch from pg/remove_verbose_logs to master March 15, 2024 08:20
@deuszx
Copy link

deuszx commented Mar 15, 2024

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
Copy link
Contributor Author

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 pgherveou changed the base branch from master to pg/zepter March 15, 2024 10:46
Base automatically changed from pg/zepter to master March 15, 2024 11:04
Copy link
Collaborator

@cmichi cmichi left a comment

Choose a reason for hiding this comment

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

Making the comment style consistent with the codebase.

crates/e2e/sandbox/src/macros.rs Outdated Show resolved Hide resolved
crates/e2e/sandbox/src/macros.rs Outdated Show resolved Hide resolved
crates/e2e/sandbox/src/macros.rs Outdated Show resolved Hide resolved
crates/e2e/sandbox/src/macros.rs Outdated Show resolved Hide resolved
crates/e2e/sandbox/src/macros.rs Outdated Show resolved Hide resolved
crates/e2e/sandbox/src/macros.rs Outdated Show resolved Hide resolved
crates/e2e/sandbox/src/macros.rs Outdated Show resolved Hide resolved
crates/e2e/sandbox/src/macros.rs Outdated Show resolved Hide resolved
crates/e2e/sandbox/src/macros.rs Outdated Show resolved Hide resolved
pgherveou and others added 3 commits March 15, 2024 12:34
@pgherveou
Copy link
Contributor Author

drink! side of the migration inkdevhub/drink#119

@pgherveou
Copy link
Contributor Author

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
Copy link

deuszx commented Mar 19, 2024

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

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

Thank you @pmikolajczyk41 @deuszx ❤️

@pgherveou
Copy link
Contributor Author

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
1175740

@pgherveou pgherveou enabled auto-merge (squash) March 19, 2024 13:35
RELEASES_CHECKLIST.md Outdated Show resolved Hide resolved
@pgherveou pgherveou merged commit fd4ab57 into master Mar 19, 2024
24 checks passed
@pgherveou pgherveou deleted the pg/move-drink branch March 19, 2024 15:30
@cmichi cmichi mentioned this pull request Nov 28, 2024
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.

5 participants