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.

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

Destructing staker proxy via ost composer contract #777

Merged
merged 3 commits into from
Jun 25, 2019

Conversation

0xsarvesh
Copy link
Contributor

@0xsarvesh 0xsarvesh commented Jun 25, 2019

fixes #771

This PR updates the flow of self-destructing staker proxy contract. With this PR, staker(owner) can call destructStakerProxy on composer contract.

This PR also delete ComposerInterface used in stakerProxy contract as there is no method call from staker proxy to composer.

@pgev pgev self-assigned this Jun 25, 2019
Copy link
Contributor

@pgev pgev left a comment

Choose a reason for hiding this comment

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

@sarvesh-ost thanks for changes. Please, have a look into my comments.

test/gateway/staker_proxy/self_destruct.js Show resolved Hide resolved
contracts/gateway/OSTComposer.sol Outdated Show resolved Hide resolved
contracts/gateway/StakerProxy.sol Show resolved Hide resolved
contracts/test/gateway/MockStakerProxy.sol Outdated Show resolved Hide resolved
@@ -0,0 +1,49 @@
// Copyright 2019 OpenST Ltd.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would propose to test the followings in addition:

  • That contract was actually selfdestruct -ed. (web3.eth.getCode(address) can be helpful I would assume).
  • That remaining ether was sent to the owner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can add contract code assertion, to ensure that selfdestruct is called. Asserting ether will be more like testing solidity selfdestruct works fine or not. Also, I don't think this contract can hold ether. There is not payable function.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've thought, like this:
we do: selfdestruct(owner) and we want to test that, and not that we do selfdestruct() or selfdestruct(non-owner-address). What do you think?

@pgev pgev removed their assignment Jun 25, 2019
@pgev pgev self-assigned this Jun 25, 2019
Copy link
Contributor

@pgev pgev left a comment

Choose a reason for hiding this comment

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

Looks great. Approved.

@pgev pgev merged commit a7065e9 into OpenST:develop Jun 25, 2019
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.

OSTComposer::removeStakerProxy should call on StakerProxy::selfDestruct
2 participants