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.

Sign up for GitHub

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

Jump to bottom

Replace default off-chain engine with experimental one, remove old one #1144

Merged
merged 11 commits into from
Mar 2, 2022
Merged

Replace default off-chain engine with experimental one, remove old one #1144

merged 11 commits into from
Mar 2, 2022

Conversation

cmichi
Copy link
Collaborator

@cmichi cmichi commented Feb 23, 2022

Follow-up to #1143.

The experimental off-chain env is currently limited in that it only supports ink_env::DefaultEnvironment, but IMHO the situation of us currently having two off-chain testing engines in every examples is worse.

@cmichi cmichi requested a review from athei February 23, 2022 05:35
@cmichi cmichi requested review from a team, Robbepop, ascjones and HCastano as code owners February 23, 2022 05:35
@cmichi cmichi force-pushed the cmichi-remove-old-off-chain-testing-env branch from 7771516 to e5bf08c Compare February 23, 2022 05:40
@cmichi cmichi force-pushed the cmichi-remove-old-off-chain-testing-env branch from e5bf08c to 3dfc2c1 Compare February 23, 2022 05:42
@cmichi cmichi marked this pull request as draft February 23, 2022 05:51
athei
athei approved these changes Feb 24, 2022
View reviewed changes
Copy link
Contributor

@athei athei left a comment

Choose a reason for hiding this comment

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

I agree. I think it is fine that it only supports the default env. The testing environment isn't a 100% match with the on chain environment anyways.

@HCastano
Copy link
Contributor

−4,528

❤️

@cmichi cmichi marked this pull request as ready for review February 25, 2022 19:59
HCastano
HCastano reviewed Mar 1, 2022
View reviewed changes
Copy link
Contributor

@HCastano HCastano left a comment

Choose a reason for hiding this comment

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

Two small questions. Once the CI is fixed it should be good to go

crates/env/src/backend.rs
@@ -46,7 +46,7 @@ impl ReturnFlags {
}

/// Returns the underlying `u32` representation.
#[cfg(not(feature = "ink-experimental-engine"))]
#[cfg(not(feature = "std"))]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this need to be behind a feature flag?

Copy link
Collaborator Author

@cmichi cmichi Mar 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

In std the function is never used, clippy fails in std otherwise.

examples/erc20/lib.rs Outdated
}
}

struct PrefixedValue<'a, 'b, T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why's this needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's used calculate the event topic hash, I've added a comment from trait-erc20 which uses the same structure.

Yeah, I'm also not happy with that and the way the tests are currently structured. Maybe we can do some broader refactoring as follow-ups.

@paritytech-cicd-pr
Copy link

🦑 📈 ink! Example Contracts ‒ Changes Report 📉 🦑

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

Δ Optimized Size Δ Used Gas Total Optimized Size Total Used Gas
accumulator 1.28 K
adder 2.23 K
contract-terminate 1.23 K 214_418
contract-transfer 8.37 K 14_418
delegator 6.24 K 46_504
dns 9.62 K 43_254
erc1155 27.49 K 86_508
erc20 9.09 K 43_254
erc721 14.37 K 115_344
flipper 1.55 K 14_418
incrementer 1.49 K 14_418
multisig 26.12 K 94_107
proxy 2.98 K 29_508
rand-extension 4.38 K 14_418
subber 2.24 K
trait-erc20 9.36 K 43_254
trait-flipper 1.33 K 14_418
trait-incrementer 1.42 K 28_836

Link to the run | Last update: Wed Mar 2 07:56:03 CET 2022

@cmichi cmichi merged commit 0d2cc5b into master Mar 2, 2022
@cmichi cmichi deleted the cmichi-remove-old-off-chain-testing-env branch March 2, 2022 07:53
@cmichi cmichi mentioned this pull request Mar 2, 2022
Merged
@xgreenx
Copy link
Collaborator

xgreenx commented Mar 2, 2022

Why minimum_balance in the test is 42 when balance of the current contract is 1000000=)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HCastano HCastano HCastano left review comments

@athei athei athei approved these changes

@Robbepop Robbepop Awaiting requested review from Robbepop

@ascjones ascjones Awaiting requested review from ascjones ascjones is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cmichi @HCastano @paritytech-cicd-pr @xgreenx @athei