Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

seal_reentrant_count returns contract reentrant count #11539

Closed
wants to merge 32 commits into from
Closed

seal_reentrant_count returns contract reentrant count #11539

wants to merge 32 commits into from

Conversation

yarikbratashchuk
Copy link
Contributor

This PR addresses #11082.

agryaznov
agryaznov suggested changes Jun 1, 2022
View reviewed changes
Copy link
Contributor

@agryaznov agryaznov left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! Here is the first review round. Also please try to make the CI green.

frame/contracts/fixtures/reentrant_count_call.wat Outdated Show resolved Hide resolved
frame/contracts/fixtures/reentrant_count_call.wat Outdated Show resolved Hide resolved
frame/contracts/src/wasm/runtime.rs Outdated Show resolved Hide resolved
frame/contracts/src/wasm/runtime.rs Outdated Show resolved Hide resolved
frame/contracts/src/wasm/mod.rs Outdated Show resolved Hide resolved
frame/contracts/src/tests.rs Outdated Show resolved Hide resolved
frame/contracts/src/tests.rs Show resolved Hide resolved
frame/contracts/fixtures/reentrant_count_call.wat Outdated Show resolved Hide resolved
frame/contracts/src/exec.rs Outdated Show resolved Hide resolved
agryaznov
agryaznov suggested changes Jun 1, 2022
View reviewed changes
frame/contracts/src/wasm/runtime.rs Outdated Show resolved Hide resolved
@xgreenx xgreenx mentioned this pull request Jun 10, 2022
Open
agryaznov
agryaznov suggested changes Jun 12, 2022
View reviewed changes
frame/contracts/src/exec.rs Outdated Show resolved Hide resolved
frame/contracts/fixtures/reentrant_count_call.wat Outdated Show resolved Hide resolved
frame/contracts/src/wasm/mod.rs Outdated Show resolved Hide resolved
frame/contracts/src/wasm/mod.rs Outdated Show resolved Hide resolved
frame/contracts/src/wasm/mod.rs Outdated Show resolved Hide resolved
frame/contracts/fixtures/reentrant_count_call.wat Outdated Show resolved Hide resolved
frame/contracts/fixtures/reentrant_count_call.wat Outdated Show resolved Hide resolved
frame/contracts/fixtures/reentrant_count_call.wat Outdated Show resolved Hide resolved
frame/contracts/fixtures/reentrant_count_call.wat Outdated Show resolved Hide resolved
frame/contracts/fixtures/reentrant_count_call.wat Outdated Show resolved Hide resolved
athei
athei reviewed Jun 17, 2022
View reviewed changes
Copy link
Member

@athei athei left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please consider using some tooling to format your wat files if you can't do it by hand. This is also way too light on tests. It just tests the bare minimum happy case. The interaction with delegate calls needs to be tested.

frame/contracts/src/wasm/runtime.rs Outdated Show resolved Hide resolved
frame/contracts/src/wasm/runtime.rs Outdated Show resolved Hide resolved
frame/contracts/src/exec.rs Outdated Show resolved Hide resolved
frame/contracts/src/exec.rs Outdated Show resolved Hide resolved
frame/contracts/src/wasm/runtime.rs Outdated Show resolved Hide resolved
frame/contracts/fixtures/reentrant_count_call.wat Outdated Show resolved Hide resolved
frame/contracts/fixtures/reentrant_count_call.wat Outdated Show resolved Hide resolved
frame/contracts/fixtures/reentrant_count_call.wat Outdated Show resolved Hide resolved
frame/contracts/fixtures/account_entrance_count_call.wat Outdated Show resolved Hide resolved
frame/contracts/src/benchmarking/mod.rs Outdated Show resolved Hide resolved
@athei athei added A0-please_review Pull request needs code review. labels Jun 24, 2022
@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Oct 11, 2022
@stale stale bot closed this Oct 26, 2022
@agryaznov agryaznov reopened this Oct 30, 2022
agryaznov
agryaznov reviewed Oct 30, 2022
View reviewed changes
frame/contracts/src/wasm/runtime.rs Outdated Show resolved Hide resolved
frame/contracts/fixtures/account_entrance_count_call.wat Outdated Show resolved Hide resolved
frame/contracts/src/tests.rs Outdated Show resolved Hide resolved
frame/contracts/fixtures/account_entrance_count_call.wat Outdated Show resolved Hide resolved
frame/contracts/fixtures/reentrant_count_delegated_call.wat Outdated Show resolved Hide resolved
frame/contracts/fixtures/account_entrance_count_call.wat Outdated
(func (export "call")
(local $account_entrance_count i32)

;; Reading "callee" contract address (which is the address of the caller)
Copy link
Contributor

Choose a reason for hiding this comment

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

I meant here that the way the contract fixture is being called, and hence the expected state of the call stack - this all should be set in the outer unit test, and no assumption on this regard should be hard-coded into the fixture. Just return the account_entrance_count with $seal_return and check it outside the contract.

agryaznov
agryaznov suggested changes Oct 30, 2022
View reviewed changes
Copy link
Contributor

@agryaznov agryaznov left a comment

Choose a reason for hiding this comment

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

Please merge master and address the highlighted issues (just a few left).

@stale stale bot removed A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. labels Oct 30, 2022
@Artemka374
Copy link
Contributor

Artemka374 commented Nov 7, 2022 •
edited
Loading

Could you review again please? I updated everything, should be ok

agryaznov
agryaznov reviewed Nov 9, 2022
View reviewed changes
frame/contracts/fixtures/reentrant_count_delegated_call.wat Outdated Show resolved Hide resolved
frame/contracts/fixtures/reentrant_count_delegated_call.wat Outdated Show resolved Hide resolved
frame/contracts/fixtures/reentrant_count_delegated_call.wat Outdated Show resolved Hide resolved
frame/contracts/fixtures/reentrant_count_delegated_call.wat Outdated Show resolved Hide resolved
frame/contracts/fixtures/reentrant_count_delegated_call.wat Outdated Show resolved Hide resolved
frame/contracts/src/benchmarking/mod.rs Outdated Show resolved Hide resolved
frame/contracts/src/benchmarking/mod.rs Outdated Show resolved Hide resolved
frame/contracts/fixtures/reentrant_count_delegated_call.wat Outdated Show resolved Hide resolved
frame/contracts/fixtures/reentrant_count_call.wat Outdated Show resolved Hide resolved
frame/contracts/fixtures/account_entrance_count_call.wat Outdated Show resolved Hide resolved
@Artemka374
Copy link
Contributor

fixed

agryaznov
agryaznov approved these changes Nov 10, 2022
View reviewed changes
Copy link
Contributor

@agryaznov agryaznov left a comment

Choose a reason for hiding this comment

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

could you please satisfy the CI?

@agryaznov
Copy link
Contributor

/benchmark runtime pallet pallet_contracts

@athei
Copy link
Member

athei commented Nov 10, 2022

/cmd queue -c bench-bot $ pallet dev pallet_contracts

@command-bot
Copy link

command-bot bot commented Nov 10, 2022 •
edited
Loading

@athei https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2033704 was started for your command "$PIPELINE_SCRIPTS_DIR/bench-bot.sh" pallet dev pallet_contracts. Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment /cmd cancel 34-7f834d54-341a-42a3-b7bf-50b7d69d8e36 to cancel this command or /cmd cancel to cancel all commands in this pull request.

@athei
Copy link
Member

athei commented Nov 10, 2022

@cr4pt0 Can you please sign the CLA or we can't merge? Or anyone rewrite the branch to remove the commit in question.

@command-bot
Copy link

command-bot bot commented Nov 10, 2022

@athei Command "$PIPELINE_SCRIPTS_DIR/bench-bot.sh" pallet dev pallet_contracts has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2033704 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2033704/artifacts/download.

@Artemka374
Copy link
Contributor

we can't get the signature right now, because this person is not working with us now, so I opened another pull request with all these changes.

@Artemka374
Copy link
Contributor

CI is satisfied there, just need some approvals again

@athei
Copy link
Member

athei commented Nov 11, 2022

Replaced by #12695

@athei athei closed this Nov 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Artemka374 Artemka374 Artemka374 left review comments

@agryaznov agryaznov agryaznov approved these changes

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

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit.
Projects
Smart Contracts
Status: Done ✅
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@yarikbratashchuk @athei @joao-paulo-parity @Artemka374 @agryaznov @RustNinja