Implement `seal_is_contract` and `seal_caller_is_origin` by agryaznov · Pull Request #1129 · use-ink/ink

agryaznov · 2022-02-09T19:00:32Z

Follow-up for paritytech/substrate#10789.

Needed for #804.

…act with self.env()

xgreenx · 2022-02-09T20:41:43Z

Hmm, I didn't notice the initial PR in the substrate so I didn't comment there. But maybe better to implement the function that returns origin. Like seal_origin and if the user wants to check he can compare it with caller.

HCastano

It looks like it works, but needs a bit of cleanup

…ntract

…anged thing

agryaznov · 2022-02-15T20:18:15Z

It looks like it works, but needs a bit of cleanup

@HCastano thanks for your review! Here comes the cleaned-up version

athei · 2022-02-15T23:02:30Z

Some notes:

It is best to not force push your branch once you asked for reviews. It makes it hard to find out which parts you already reviewed.
Please make sure to satisfy the CI (except waterfall).

athei · 2022-02-17T10:28:37Z

Hmm, I didn't notice the initial PR in the substrate so I didn't comment there. But maybe better to implement the function that returns origin. Like seal_origin and if the user wants to check he can compare it with caller.

I think we should have both functions.

xgreenx · 2022-02-17T10:57:11Z

I think we should have both functions.

Agree. When is possible we will use cheaperseal_caller_is_origin, else seal_origin.

HCastano · 2022-02-17T17:54:00Z

Please make sure to satisfy the CI (except waterfall).

Please do fix any ink-waterfall issues, we fixed the CI staged a week or two ago 😄

athei · 2022-02-17T17:57:48Z

Please make sure to satisfy the CI (except waterfall).

Please do fix any ink-waterfall issues, we fixed the CI staged a week or two ago 😄

But I think the node version used might be too old if the new features are used in the examples? How do I update the node there? It appears to be installed in the CI image?

cmichi · 2022-02-17T18:16:53Z

But I think the node version used might be too old if the new features are used in the examples? How do I update the node there? It appears to be installed in the CI image?

The substrate-contracts-node in the CI's Docker image is updated nightly: https://github.com/paritytech/scripts/blob/master/dockerfiles/contracts-ci-linux/Dockerfile#L53-L56. So should be fine. And CI on this PR already fails earlier.

cmichi · 2022-02-17T18:19:01Z

@agryaznov You should see the CI output if you click on "Details" right of the failed jobs. You can also reproduce it locally, this list here should give some hints: https://github.com/paritytech/ink/blob/master/CONTRIBUTING.md#checklist (it might be a bit outdated though, so better ask before trying to figure out why something isn't working).

athei · 2022-02-17T18:43:00Z

But I think the node version used might be too old if the new features are used in the examples? How do I update the node there? It appears to be installed in the CI image?

The substrate-contracts-node in the CI's Docker image is updated nightly: https://github.com/paritytech/scripts/blob/master/dockerfiles/contracts-ci-linux/Dockerfile#L53-L56. So should be fine. And CI on this PR already fails earlier.

Is it build with contracts-unstable-interface?

cmichi · 2022-02-17T19:45:13Z

But I think the node version used might be too old if the new features are used in the examples? How do I update the node there? It appears to be installed in the CI image?

The substrate-contracts-node in the CI's Docker image is updated nightly: https://github.com/paritytech/scripts/blob/master/dockerfiles/contracts-ci-linux/Dockerfile#L53-L56. So should be fine. And CI on this PR already fails earlier.

Is it build with contracts-unstable-interface?

Yup: https://github.com/paritytech/substrate-contracts-node/blob/main/runtime/Cargo.toml#L58-L63.

…ntract

This reverts commit a40fab1.

codecov-commenter · 2022-02-18T10:57:10Z

Codecov Report

Merging #1129 (7d463d6) into master (07a8ed9) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1129   +/-   ##
=======================================
  Coverage   78.69%   78.69%           
=======================================
  Files         252      252           
  Lines        9395     9395           
=======================================
  Hits         7393     7393           
  Misses       2002     2002

Impacted Files	Coverage Δ
crates/env/src/api.rs	`36.36% <ø> (ø)`
crates/env/src/backend.rs	`80.64% <ø> (ø)`
...tes/env/src/engine/experimental_off_chain/impls.rs	`53.70% <ø> (ø)`
crates/env/src/engine/off_chain/impls.rs	`41.88% <ø> (ø)`
crates/lang/src/env_access.rs	`12.00% <ø> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07a8ed9...7d463d6. Read the comment docs.

paritytech-cicd-pr · 2022-02-18T11:11:57Z

🦑 📈 ink! Example Contracts ‒ Changes Report 📉 🦑

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

	Total Optimized Size	Total Used Gas
`accumulator`	1.28 K
`adder`	2.23 K
`contract-terminate`	1.23 K	215_704
`contract-transfer`	8.37 K	15_704
`delegator`	6.26 K	33_142
`dns`	9.64 K	47_112
`erc1155`	27.71 K	94_224
`erc20`	9.12 K	47_112
`erc721`	14.40 K	125_632
`flipper`	1.57 K	15_704
`incrementer`	1.49 K	15_704
`multisig`	26.38 K	103_006
`proxy`	2.98 K	32_082
`rand-extension`	4.40 K	15_704
`subber`	2.24 K
`trait-erc20`	9.39 K	47_112
`trait-flipper`	1.35 K	15_704
`trait-incrementer`	1.45 K	31_408

Link to the run | Last update: Wed Feb 23 12:27:29 CET 2022

agryaznov · 2022-02-18T11:48:16Z

CI issues seem to be fixed now.
Have to suppress wrong_self_convention clippy check or to rename is_contract function.
The first option is chosen for now.

cmichi

I've got only some nitpicks, looks good besides that, we can merge after those are fixed.

Could you click "Resolve conversation" for the comments by reviewers which were resolved? That makes it easier to follow what's going on. Thanks!

Co-authored-by: Michael Müller <mich@elmueller.net>

…ntract

…mental_off_chain eingine

HCastano · 2022-02-22T17:14:47Z

Will take a look at this again today, sorry!

agryaznov · 2022-02-22T17:29:04Z

I'm not sure why ink-waterfall ci now fails.

It began to fall with an error starting from this commit which changes nothing apart from comment typos.

cmichi · 2022-02-22T18:18:20Z

I'm not sure why ink-waterfall ci now fails.

It began to fall with an error starting from this commit which changes nothing apart from comment typos.

It's not your fault, it's because we currently don't use fixed nightly versions anywhere in CI. Since yesterday the Rust nightly version contains a bug which makes cargo-contract fail. You can also see it if you take a look at the CI on master for ink and cargo-contract. Today a commit was merged into Rust which I hope will fix this, so if we're lucky the master CI should work everywhere again tomorrow (every night there is an automatic run of the CI's on master with the latest Rust nightly).

For this PR we can ignore the ink-waterfall for now.

cmichi

Nice, thanks!

HCastano

Couple small things, but looks good!

My take on resolving comments: as a reviewer I want to be the one to mark comments as resolved. That way I'm able to get a refresher on what comments I've made, and judge whether or not they were actually resolved correctly

HCastano · 2022-02-19T22:56:38Z

+    /// pub fn is_contract(&mut self, account_id: AccountId) -> bool {
+    ///     self.env().is_contract(&account_id)
+    /// }


Suggested change

/// pub fn is_contract(&mut self, account_id: AccountId) -> bool {

/// self.env().is_contract(&account_id)

/// }

/// pub fn is_contract(&mut self, account_id: &AccountId) -> bool {

/// self.env().is_contract(account_id)

/// }

Hm, no, this won't work as Ink messages can't take references as params.

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>

athei · 2022-02-23T08:08:40Z

The other tests need to be green, though :)

…ntract

athei

LGTM. You can merge once the CI is green.

agryaznov added 6 commits February 9, 2022 19:42

[env][lang] add support for new is_contract API

b817ff8

added is_contract() to ink_lang::EnvAccess to use from within contr…

9c39937

…act with self.env()

added is-contract example

2a58c38

add support for caller_is_origin() API

5e1bc60

updated is-contract example to use caller_is_origin() as well

ff08823

Merge branch 'master' of github.com:agryaznov/ink into is_contract

623b044

agryaznov requested review from HCastano, Robbepop, ascjones and cmichi as code owners February 9, 2022 19:00

agryaznov changed the title ~~Implement 'seal_is_contract and seal_caller_is_origin`~~ Implement seal_is_contract and seal_caller_is_origin Feb 9, 2022

athei linked an issue Feb 9, 2022 that may be closed by this pull request

Is there a way to check if an AccountId is a contract? #804

Closed

HCastano reviewed Feb 14, 2022

View reviewed changes

agryaznov added 3 commits February 15, 2022 21:50

Corrections in response to @HCastrano review

b1310fb

Merge branch 'master' of https://github.com/paritytech/ink into is_co…

81ec866

…ntract

removed examples/is-contract

444c497

agryaznov force-pushed the is_contract branch from 3d71b32 to 02ca62a Compare February 15, 2022 19:27

turned back whitespaces in env_access.rs not to mess up with the ch…

5617aeb

…anged thing

agryaznov force-pushed the is_contract branch from 02ca62a to 5617aeb Compare February 15, 2022 20:17

agryaznov added 4 commits February 18, 2022 11:55

Merge branch 'master' of https://github.com/paritytech/ink into is_co…

0bfcd80

…ntract

added new functions to experimental_offchain_engine

4f76c30

fixed wrong_self_convention clippy warning for is_ functions

a40fab1

supressed wrong_self_convention clippy warning for is_ functions

123474e

This reverts commit a40fab1.

athei reviewed Feb 18, 2022

View reviewed changes

Comment thread crates/env/src/engine/experimental_off_chain/impls.rs

cmichi reviewed Feb 22, 2022

View reviewed changes

Comment thread crates/env/src/api.rs Outdated

cmichi reviewed Feb 22, 2022

View reviewed changes

Comment thread crates/env/src/engine/off_chain/impls.rs

cmichi requested changes Feb 22, 2022

View reviewed changes

agryaznov and others added 3 commits February 22, 2022 17:20

Update crates/env/src/api.rs

6ac81a7

Co-authored-by: Michael Müller <mich@elmueller.net>

Merge branch 'master' of https://github.com/paritytech/ink into is_co…

f5d9384

…ntract

set is_contract and caller_is_origin as unimplemented in experi…

68d7cc4

…mental_off_chain eingine

cmichi approved these changes Feb 22, 2022

View reviewed changes

HCastano approved these changes Feb 22, 2022

View reviewed changes

Apply suggestions from code review

b00e7e4

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>

agryaznov added 2 commits February 23, 2022 13:55

message cant take ref as a param: revert suggested change

2061940

Merge branch 'master' of https://github.com/paritytech/ink into is_co…

7d463d6

…ntract

athei approved these changes Feb 23, 2022

View reviewed changes

agryaznov merged commit ff5add1 into use-ink:master Feb 23, 2022

Conversation

agryaznov commented Feb 9, 2022

Uh oh!

xgreenx commented Feb 9, 2022

Uh oh!

HCastano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

agryaznov commented Feb 15, 2022

Uh oh!

athei commented Feb 15, 2022

Uh oh!

athei commented Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xgreenx commented Feb 17, 2022 • edited by athei Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HCastano commented Feb 17, 2022

Uh oh!

athei commented Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmichi commented Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmichi commented Feb 17, 2022

Uh oh!

athei commented Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmichi commented Feb 17, 2022

Uh oh!

codecov-commenter commented Feb 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

paritytech-cicd-pr commented Feb 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦑 📈 ink! Example Contracts ‒ Changes Report 📉 🦑

Uh oh!

agryaznov commented Feb 18, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cmichi left a comment

Choose a reason for hiding this comment

Uh oh!

HCastano commented Feb 22, 2022

Uh oh!

agryaznov commented Feb 22, 2022

Uh oh!

cmichi commented Feb 22, 2022

Uh oh!

cmichi left a comment

Choose a reason for hiding this comment

Uh oh!

HCastano left a comment

Choose a reason for hiding this comment

Uh oh!

HCastano Feb 19, 2022

Choose a reason for hiding this comment

Uh oh!

agryaznov Feb 23, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

athei commented Feb 17, 2022 •

edited

Loading

xgreenx commented Feb 17, 2022 •

edited by athei

Loading

athei commented Feb 17, 2022 •

edited

Loading

cmichi commented Feb 17, 2022 •

edited

Loading

athei commented Feb 17, 2022 •

edited

Loading

codecov-commenter commented Feb 18, 2022 •

edited

Loading

paritytech-cicd-pr commented Feb 18, 2022 •

edited

Loading