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

contracts: Remove OnKilledAccount implementation #5397

Merged
merged 3 commits into from
Mar 26, 2020
Merged

Conversation

athei
Copy link
Member

@athei athei commented Mar 25, 2020

Contracts now longer rely on this callback to tell them when they are removed. Instead, they can only self destruct using ext_terminate.

Even without this change the now removed implementation wasn't called anymore in our default substrate runtime.

closes #4952

@athei athei added A0-please_review Pull request needs code review. M8-contracts B0-silent Changes should not be mentioned in any release notes labels Mar 25, 2020
@athei athei requested a review from pepyakin March 25, 2020 11:28
@athei athei requested a review from gui1117 as a code owner March 25, 2020 11:28
@parity-cla-bot
Copy link

It looks like @athei signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

pepyakin
pepyakin previously approved these changes Mar 25, 2020
@pepyakin pepyakin dismissed their stale review March 25, 2020 12:12

Just realized that the test-linux failure is actually legit. Needs addressing

@athei
Copy link
Member Author

athei commented Mar 25, 2020

Yes I am already on it. The two failing tests are:

self_destruct_works

This is legit and might be caused by the early exit in AccountsDB::commit which should be removed (at least the test is green with it removed). I still do not understand why because the destruction and the account balance should be in different changesets? I might be wrong.

account_removal_removes_storage

I think this test makes no longer sense as as we specifically removed this functionality. Only ext_terminate can remove the storage.

@athei
Copy link
Member Author

athei commented Mar 25, 2020

Turns out that AccountDB merges the change sets which makes the early exit in AccountsDB::commit incorrect with the removal of OnKilledAccount. I removed the check.

athei added 3 commits March 25, 2020 14:54
Contracts now longer rely on this callback to tell them when they
are removed. Instead, they can only self destruct  using `ext_terminate`.
@athei athei force-pushed the at-contract-removal branch from c585935 to cf8558e Compare March 25, 2020 14:05
@athei
Copy link
Member Author

athei commented Mar 25, 2020

The CI fail seems unrelated to the change. @pepyakin Any idea why this is failing?

@pepyakin
Copy link
Contributor

Apparently, this has something to do with #5366. I am not sure though what to do about it, except waiting for paritytech/polkadot#938 to be landed.

// This should lead to the removal of all storage associated with this account.
// This does not remove the contract storage as we are not notified about a
// account removal. This cannot happen in reality because a contract can only
// remove itself by `ext_terminate`. There is no external event that can remove
Copy link
Contributor

Choose a reason for hiding this comment

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

And because we have an assumption that the contract's account is only controlled by that contract.

Copy link
Contributor

@pepyakin pepyakin left a comment

Choose a reason for hiding this comment

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

Looks good.

@pepyakin pepyakin added A8-mergeoncegreen and removed A0-please_review Pull request needs code review. labels Mar 25, 2020
@kianenigma
Copy link
Contributor

Apparently, this has something to do with #5366. I am not sure though what to do about it, except waiting for paritytech/polkadot#938 to be landed.

yea, should be fixed now.

@gavofyork gavofyork merged commit 58439b5 into master Mar 26, 2020
@gavofyork gavofyork deleted the at-contract-removal branch March 26, 2020 10:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
B0-silent Changes should not be mentioned in any release notes
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

srml-contracts: ext_terminate
6 participants