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

Identity transfer eth #145

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 31 additions & 9 deletions contracts/Identity.sol
Original file line number Diff line number Diff line change
Expand Up @@ -64,24 +64,46 @@ contract Identity is KeyManager {
address to,
uint256 value,
bytes memory data
)
) onlyAction
public
payable
Copy link
Member

Choose a reason for hiding this comment

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

🥇

returns (bool success)
{

bytes32 key_ = addressToKey(msg.sender);
require(
keyHasPurpose(key_, ACTION) && _keys[key_].revokedAt == 0,
"Requester must have an ACTION purpose"
);

// 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)
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

If the whole transaction fails I am fine with it. :)

}
}
/**
* @dev Transfer eth
* @param to address of account where to transfer eth.
* it can not be a contract. Use execute method in that case
* @param value uint256 wei supply for proxy execution
* @param data bytes ABI encoded call data
*/
function transferEth(
Copy link
Member

@xmxanuel xmxanuel Apr 12, 2019

Choose a reason for hiding this comment

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

Don't we need a payable fallback function in the identity contract if we are dealing with ETH?

address to,
uint256 value,
bytes memory data
) onlyAction
public
payable
returns (bool success)
{
// solium-disable-next-line security/no-inline-assembly
assembly {
// Make sure the address is not a contract
if gt(extcodesize(to),0) {
revert(0, 0)
Copy link
Member

Choose a reason for hiding this comment

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

Why not calling the execute method in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not understand you quesiton

Copy link
Member

Choose a reason for hiding this comment

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

In case a user wants to send ETH to a contract. The user should use the execute method.

Suggested change
revert(0, 0)
success := execute(...)

Copy link
Member

Choose a reason for hiding this comment

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

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 :)

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 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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Anyway, it is a more general discussion. Overall PR looks good. If a payable keyword is not needed and ETH is transferred.

}
success := call(gas, to, value, add(data, 0x20), mload(data), 0, 0)
}
}



}
14 changes: 13 additions & 1 deletion contracts/KeyManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,19 @@ contract KeyManager {
bytes32 key_ = addressToKey(msg.sender);
require(
keyHasPurpose(key_, MANAGEMENT) && _keys[key_].revokedAt == 0,
"No management right"
"Sender must have MANAGEMENT purpose"
);
_;
}

/**
* @dev Throws if called by any account other than a ACTION key.
*/
modifier onlyAction() {
Copy link
Member

Choose a reason for hiding this comment

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

👍

bytes32 key_ = addressToKey(msg.sender);
require(
keyHasPurpose(key_, ACTION) && _keys[key_].revokedAt == 0,
"Sender must have ACTION purpose"
);
_;
}
Expand Down
18 changes: 18 additions & 0 deletions contracts/mock/TestProxyExecution.sol
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,36 @@ contract TextProxyExecution {

// Counts calls by msg.sender
mapping(address => uint) public numCalls;
mapping(address => uint) public paidCalls;

function callMe()
external
payable
{
numCalls[msg.sender] += 1;
}

function callMeWithMoney()
external
payable
{
require(msg.value > 0);
paidCalls[msg.sender] += 1;
}

function getCallsFrom(address caller)
external
view
returns (uint noOfCalls)
{
return numCalls[caller];
}

function getPayedCallsFrom(address caller)
external
view
returns (uint noOfCalls)
{
return paidCalls[caller];
}
}
Loading