Identity transfer eth #145
Conversation
| * @param value uint256 wei supply for proxy execution | ||
| * @param data bytes ABI encoded call data | ||
| */ | ||
| function transferEth( |
There was a problem hiding this comment.
Don't we need a payable fallback function in the identity contract if we are dealing with ETH?
test/Identity.test.js
Outdated
| const numOfCalls = await this.testProxy.getCallsFrom(this.identity.address); | ||
| assert.equal(1, numOfCalls.toNumber()); | ||
| }) | ||
|
|
||
|
|
||
| it('should transferEth using the a identity ACTION key', async function () { | ||
| await this.identity.addKey(addressToBytes32(accounts[1]), ACTION, 1); | ||
| await shouldSucceed(this.identity.transferEth(accounts[2], 1, "0x1234", {from: accounts[1]})); |
There was a problem hiding this comment.
Do you check if account[2] has one more ETH on his account after the method call?
| // solium-disable-next-line security/no-inline-assembly | ||
| assembly { | ||
| // check if contract to be call exists | ||
| // check if contract to be called exists | ||
| if iszero(extcodesize(to)) { | ||
| revert(0, 0) | ||
| } | ||
| success := call(gas, to, value, add(data, 0x20), mload(data), 0, 0) |
There was a problem hiding this comment.
What happens if the contract I want to send some ETH is not payable?
Would the complete transaction fail or would the identity contract own the money?
In case the identity contract would own it.
Is it possible to withdraw the ETH from the identity contract?
There was a problem hiding this comment.
You should know what contract you are calling. The identity contract is just a proxy and does not care about how the contract is calling is implemented. Funds on the identity contract is not something we support and I think it a broader discussion.
There was a problem hiding this comment.
If the whole transaction fails I am fine with it. :)
| assembly { | ||
| // Make sure the address is not a contract | ||
| if gt(extcodesize(to),0) { | ||
| revert(0, 0) |
There was a problem hiding this comment.
Why not calling the execute method in that case?
There was a problem hiding this comment.
I do not understand you quesiton
There was a problem hiding this comment.
In case a user wants to send ETH to a contract. The user should use the execute method.
| revert(0, 0) | |
| success := execute(...) |
There was a problem hiding this comment.
Ok. but calling the execute method would result in the same line of code in the end:
success := call(gas, to, value, add(data, 0x20), mload(data), 0, 0)
Maybe I am questioning more the if condition itself :)
There was a problem hiding this comment.
Because I want this method to revert and force the user to use the execute api. Execute is for contract call and transferEth is for account calling. The old execute method worked like this and the idea is to be more clear and have 2 methods.
There was a problem hiding this comment.
The execute checkIfContract exists was a recommendation from ToB, if I am correct?
I don't want to change that.
In Ethereum accounts and contracts are on the same level. Let say I need to send ETH to a 1000 addresses, why should I care if the other users using an account or a contract.
Maybe I don't know it, and I would need to implement an additional check and switching between execute and transferETH for my 1000 transactions
There was a problem hiding this comment.
Yes that what you should do. You should know what addresses you are using. It the same thing if you send some Eth from metamask. The client should know if the address is correct and what is behind it. We could remove the check in the tranferEth and make it like the old execute method but then you could bypass execute and you are losing eth in the case you wanna call a contract and send it eth
There was a problem hiding this comment.
I understood the Ethereum philosophy in that way: It shouldn't matter if an address is an account or a contract. This design decision enabled a lot of cool things like multi-sig contracts etc.
In case somebody would use our identity transferETH in another contract without an additional isContract check. It would suddenly matter if an address is an account or a contract.
There was a problem hiding this comment.
Anyway, it is a more general discussion. Overall PR looks good. If a payable keyword is not needed and ETH is transferred.
| /** | ||
| * @dev Throws if called by any account other than a ACTION key. | ||
| */ | ||
| modifier onlyAction() { |
|
|
||
| it('should transferEth using the a identity ACTION key', async function () { | ||
| await this.identity.addKey(addressToBytes32(accounts[1]), ACTION, 1); | ||
| await shouldSucceed(this.identity.transferEth(accounts[2], 1, "0x1234", {from: accounts[1]})); | ||
| await this.identity.addKey(addressToBytes32(accounts[2]), ACTION, 1); |
| @@ -66,6 +66,7 @@ contract Identity is KeyManager { | |||
| bytes memory data | |||
| ) onlyAction | |||
| public | |||
| payable | |||
|
What about this PR? |
No description provided.