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

Advanced test environment #1589

Open
wants to merge 49 commits into
base: master
Choose a base branch
from

Conversation

Artemka374
Copy link

This is PR for this issue: Supercolony-net/openbrush-contracts#136 (comment)
that was started in Supercolony-net#4

It allows testing cross-contract calls in off-chain environment

  • Add support of call_flags
  • Error & revert handling

@codecov-commenter
Copy link

codecov-commenter commented Jan 16, 2023

Codecov Report

Merging #1589 (6804763) into master (858684d) will decrease coverage by 0.89%.
The diff coverage is 39.08%.

@@            Coverage Diff             @@
##           master    #1589      +/-   ##
==========================================
- Coverage   70.44%   69.56%   -0.89%     
==========================================
  Files         207      207              
  Lines        6398     6548     +150     
==========================================
+ Hits         4507     4555      +48     
- Misses       1891     1993     +102     
Impacted Files Coverage Δ
crates/engine/src/database.rs 100.00% <ø> (ø)
crates/engine/src/exec_context.rs 100.00% <ø> (ø)
crates/engine/src/types.rs 66.66% <ø> (ø)
crates/env/src/api.rs 34.78% <0.00%> (ø)
crates/env/src/backend.rs 73.52% <0.00%> (-4.60%) ⬇️
crates/env/src/engine/mod.rs 63.33% <ø> (ø)
crates/ink/codegen/src/generator/dispatch.rs 94.19% <ø> (ø)
crates/env/src/engine/off_chain/impls.rs 37.68% <20.86%> (-8.08%) ⬇️
crates/engine/src/test_api.rs 84.78% <42.85%> (-0.94%) ⬇️
crates/engine/src/ext.rs 67.24% <67.24%> (-0.74%) ⬇️
... and 8 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@HCastano
Copy link
Contributor

@Artemka374 thanks for the PR, sorry for leaving it sitting around so long.

Tbh not sure when we'll have time to review this since it's a non-trivial addition

@coreggon11
Copy link

Hi @HCastano thanks for the reply. We understand that there is so much going on in ink! right now, I will just point out that this is a demanded feature and would make development (and testing especially) much easier for devs. We will keep maintaining this PR and are happy to hear some feedback when you will have time to get to this 😄

@paritytech-cicd-pr
Copy link

🦑 📈 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 examples/* contracts from this branch with cargo-contract 2.0.0-rc-da24337 and comparing them to ink! master:

Δ Optimized Size Δ Used Gas Total Optimized Size Total Used Gas
accumulator -0.01 K 1.11 K
adder -0.07 K 1.69 K
call-runtime 1.80 K
contract-terminate -0.02 K 1.27 K 129_990
contract-transfer -0.12 K 1.33 K 129_990
custom-environment -0.03 K 2.54 K
custom_allocator 8.39 K
delegator +0.44 K 6.45 K 259_980
dns -0.16 K 8.73 K 389_970
erc1155 -0.44 K 16.78 K 779_940
erc20 -0.20 K 7.39 K 389_970
erc721 -0.23 K 11.19 K 1_039_920
flipper 1.42 K 129_990
forward-calls -0.05 K 2.40 K 259_980
incrementer -0.01 K 1.21 K
mapping_integration_tests -0.25 K 3.11 K
mother +0.13 K 10.57 K
multisig -0.49 K 23.68 K 779_940
payment-channel -0.04 K 5.97 K
psp22-extension -0.08 K 6.55 K
rand-extension -0.07 K 2.87 K
reentrancy +3.62 K 3.62 K
set-code-hash -0.09 K 1.52 K
subber -0.07 K 1.72 K
trait-erc20 -0.20 K 7.82 K 389_970
trait-flipper 1.19 K 129_990
trait-incrementer 1.35 K 259_980
updated-incrementer -0.09 K 1.53 K

Link to the run | Last update: Wed Mar 1 13:40:10 CET 2023

@cmichi
Copy link
Collaborator

cmichi commented Apr 12, 2023

@Artemka374 Could you clarify the following:

It allows testing cross-contract calls in off-chain environment

Is this the main intention for the PR? Because we have released ink! 4.0 in the meantime and E2E testing supports this by default.

The reason I'm asking is because the PR has 1k lines of code and there is some maintenance costs associated with it. We're thinking about what functionality to support in the off-chain testing engine and for which to require usage of the E2E testing.

@Artemka374
Copy link
Author

@cmichi Actually, it is more like full implementation of off-chain functionality, that wasn't accessible before, but I think off-chain cross-contract calls testing is the most important part of it.

@deuszx
Copy link

deuszx commented Aug 18, 2023

I think this is superseeded by what's being enabled with #1864 .

@ascjones
Copy link
Collaborator

I think this is superseeded by what's being enabled with #1864 .

@xgreenx has pointed out that a strong reason to keep this would be for the ability to do step by step debugging of ink contracts.

@deuszx
Copy link

deuszx commented Aug 18, 2023

I think this is superseeded by what's being enabled with #1864 .

@xgreenx has pointed out that a strong reason to keep this would be for the ability to do step by step debugging of ink contracts.

Using off-chain environment though, right? Not the actual one on which the contract will be ran after deployment.

@ascjones
Copy link
Collaborator

I think this is superseeded by what's being enabled with #1864 .

@xgreenx has pointed out that a strong reason to keep this would be for the ability to do step by step debugging of ink contracts.

Using off-chain environment though, right? Not the actual one on which the contract will be ran after deployment.

Exactly

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.

8 participants