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.

Sign up for GitHub

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

Jump to bottom

Use Address.sendValue instead of address.tranfer in RefundEscrow #2480

Merged
merged 4 commits into from
Jan 26, 2021
Merged

Use Address.sendValue instead of address.tranfer in RefundEscrow #2480

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d98e089
Use Address.sendValue instead of address.tranfer
Amxx Jan 22, 2021
80856d1
revert to "using for" library syntax for consistency
frangio Jan 26, 2021
47931c7
Merge branch 'master' into fix/RefundEscrow_sendValue
frangio Jan 26, 2021
4ad0c14
add changelog entry
frangio Jan 26, 2021
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
2 changes: 1 addition & 1 deletion contracts/payment/escrow/RefundEscrow.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ contract RefundEscrow is ConditionalEscrow {
*/
function beneficiaryWithdraw() public virtual {
require(_state == State.Closed, "RefundEscrow: beneficiary can only withdraw while closed");
_beneficiary.transfer(address(this).balance);
Address.sendValue(_beneficiary, address(this).balance);
Copy link
Contributor

Choose a reason for hiding this comment

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

Well this is a good moment to discuss, why do you prefer this syntax for using libraries?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because it avoid any ambiguity over the fact that this is a library being called, and not a builtin function. Imagine if Address contained a transfer function (good thing it doesn't) ... then what would address.transfer be ? The library, the built-in function ?

I think the using for syntax is great for safemath. I also think it is acceptable when the first type is clearly not a contract (EnumerableMap/EnumerableSet) ... For addresses I'm not sure ... for contracts I think it's a terrible idea.

In TokenTimelock there is an IERC20 token that is called using _token.safeTransfer(beneficiary(), amount);. I think there is a real ambiguity between a potential safeTransfer function included in IERC20, and a safeTransfer library function that is used for IERC20 (which is the case here).

TLDR:
In the particular case, I wouldn't oppose doing _beneficiary.sendValue(address(this).balance);, but there are cases where I think this syntax is to be avoided.

Copy link
Collaborator Author

@Amxx Amxx Jan 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I noticed that using XXX for address; and using XXX for address payable; are two separate things.

pragma solidity ^0.8.0;

library Address {
    event Sent(address,uint256);
    function transfer(address to, uint256 value) internal {
        (bool success, ) = to.call{value: value}("");
        require(success);
        emit Sent(to, value);
    }
}

contract Test {
    using Address for address;
    receive() external payable {}
    
    function send1(address payable to, uint256 value) external payable {
        to.transfer(value); // built in mechanism
    }
    
    function send2(address to, uint256 value) external payable {
        to.transfer(value); // using Library
    }
}

EDIT: in the code above, if we replace using Address for address; with using Address for address payable;, send1 compilation gives an error (member not unique).

Copy link
Contributor

Choose a reason for hiding this comment

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

They're good points. I agree that for contracts it's confusing. For this case I would leave the using for syntax for consistency with the rest of the library.

}

/**
Expand Down