Skip to content

Conversation

@orenyodfat
Copy link
Contributor

No description provided.

ben-kaufman
ben-kaufman previously approved these changes May 20, 2020
Copy link
Contributor

@dkent600 dkent600 left a comment

Choose a reason for hiding this comment

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

Why is this contract name prefixed with "dxDao"? Does it have no possible more widespread use?

// solhint-disable-next-line not-rely-on-time
require(releaseTime < now, "cannot withdraw before releaseTime");
// solhint-disable-next-line avoid-call-value
(bool success, ) = owner.call.value(address(this).balance)("");

Choose a reason for hiding this comment

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

Why not msg.sender.transfer(address(this).balance) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because transfer is limited in gas. it might work for dxdao avatar though no promise for other contracts which consume gas on the fallback function.
https://diligence.consensys.net/blog/2019/09/stop-using-soliditys-transfer-now/

public
returns(bool success, bytes memory returnValue) {
// solhint-disable-next-line avoid-low-level-calls
(success, returnValue) = _contract.call(_encodedABI);

Choose a reason for hiding this comment

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

Why instead of sending the _encodeABI we execute the withdraw function with the signature that we already know it will have?

(success, returnValue) = _contract.call(abi.encodeWithSignature("withdraw()"));

Doing this we make sure that the ONLY function that can be executed is the withdraw()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a public generic function which can call any contract specific Abis.
this is just a test helper contract and is not part of any dao.

@AugustoL
Copy link

Why is this contract name prefixed with "dxDao"? Does it have no possible more widespread use?

yeah, right? I guess the idea is to have this feature to all DAOs..

@orenyodfat
Copy link
Contributor Author

Why is this contract name prefixed with "dxDao"? Does it have no possible more widespread use?

yeah, right? I guess the idea is to have this feature to all DAOs..

I am not sure yet . We might have it specific to dxdao by hard coded its address and release time.though might complicate the tests.
yea. lets make it general

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