-
Notifications
You must be signed in to change notification settings - Fork 6
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
Switch from raw ETH to ERC20 #78
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!! Looks good to me.
(bool success, ) = address(_activePool).call{value: _amount}(""); | ||
require(success, "BorrowerOps: Sending ETH to ActivePool failed"); | ||
function _pullETHAndSendToActivePool(IActivePool _activePool, uint256 _amount) internal { | ||
// Pull ETH tokens from sender (we may save gas by pulling directly from Active Pool, but then the approval UX for user would be weird) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I sort-of remember this from the HP approach, but why would it be weird - because they have to approve a contract (ActivePool) different from the one they are interacting with (BorrowerOperations)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly.
contracts/src/BoldToken.sol
Outdated
@@ -298,6 +299,7 @@ contract BoldToken is CheckContract, IBoldToken { | |||
return _VERSION; | |||
} | |||
|
|||
// TODO: Do we need this? | |||
function permitTypeHash() external pure override returns (bytes32) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we don't need the external function.
@@ -1158,7 +1158,7 @@ contract TroveManager is LiquityBase, Ownable, CheckContract, ITroveManager { | |||
// Transfer coll and debt from ActivePool to DefaultPool | |||
_activePool.decreaseBoldDebt(_debt); | |||
_defaultPool.increaseBoldDebt(_debt); | |||
_activePool.sendETH(address(_defaultPool), _coll); | |||
_activePool.sendETHToDefaultPool(_coll); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, surprisingly few changes in TroveManager
! Nice
// import "forge-std/console.sol"; | ||
|
||
|
||
contract NonPayable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick, but it seems it's not always "NonPayable" - maybe we can rename? "NonPayableSwitch" or something?
await defaultPool.setAddresses(mockTroveManager.address, mockActivePool.address) | ||
}) | ||
|
||
it('sendETHToActivePool(): fails if receiver cannot receive ETH', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this one now redundant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, as now it’s not raw ETH, it doesn’t matter if the receiver is payable or not.
@@ -896,6 +903,32 @@ class TestHelper { | |||
}; | |||
} | |||
|
|||
static async openTroveWrapper( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice.
Updated ETH flows diagram:
Closes #73