Skip to content

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

Merged
cmichi merged 11 commits intomasterfrom
cmichi-remove-old-off-chain-testing-env
Mar 2, 2022
Merged

Replace default off-chain engine with experimental one, remove old one#1144
cmichi merged 11 commits intomasterfrom
cmichi-remove-old-off-chain-testing-env

Conversation

@cmichi
Copy link
Copy Markdown
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, HCastano, Robbepop and ascjones 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
Copy link
Copy Markdown
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
Copy Markdown
Contributor

−4,528

❤️

@cmichi cmichi marked this pull request as ready for review February 25, 2022 19:59
Copy link
Copy Markdown
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


/// Returns the underlying `u32` representation.
#[cfg(not(feature = "ink-experimental-engine"))]
#[cfg(not(feature = "std"))]
Copy link
Copy Markdown
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
Copy Markdown
Collaborator Author

@cmichi cmichi Mar 2, 2022

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.

}
}

struct PrefixedValue<'a, 'b, T> {
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown

🦑 📈 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
@xgreenx
Copy link
Copy Markdown
Contributor

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants