Reserve supports multiple exchanges #6299
Conversation
…org/celo-monorepo into martinvol/multipleExchangeSpenders
d521aa5 to
1bd660c
Compare
…org/celo-monorepo into martinvol/multipleExchangeSpenders
| @@ -311,6 +311,11 @@ contract Reserve is | |||
| emit SpenderRemoved(spender); | |||
| } | |||
|
|
|||
| function _isAllowedToSpendExchange(address spender) private view returns (bool) { | |||
There was a problem hiding this comment.
Why not making this public?
| await assertRevert(reserve.removeExchangeSpender(nonOwner, 0, { from: nonOwner })) | ||
| }) | ||
|
|
||
| // TODO should allow exchange |
…org/celo-monorepo into martinvol/multipleExchangeSpenders
…org/celo-monorepo into martinvol/multipleExchangeSpenders
|
|
||
| /** | ||
| * @notice Takes away an address's permission to spend Reserve funds without limits. | ||
| * @param spender The address that is to be no longer allowed to spend Reserve funds. |
| emit SpenderRemoved(spender); | ||
| } | ||
|
|
||
| function isAllowedToSpendExchange(address spender) public view returns (bool) { |
| emit ExchangeSpenderRemoved(spender); | ||
| } | ||
|
|
||
| function getExchangeSpenders() public view returns (address[] memory) { |
There was a problem hiding this comment.
Can we remove this? exchangeSpenderAddresses is a public variable so this is redundant
There was a problem hiding this comment.
Looks like this is only used for testing-- think it's worth removing? iirc there isn't a clean way of getting exchangeSpenderAddresses.length from web3 to properly iterate through. Because it's already in there for CR3 I don't think we would gain much now from removing it, if anything it would introduce backward incompatibility
| emit SpenderRemoved(spender); | ||
| } | ||
|
|
||
| function isAllowedToSpendExchange(address spender) public view returns (bool) { | ||
| return |
There was a problem hiding this comment.
Why is this function necessary? Please include a comment explaining why isExchangeSpender is not enough, and the conditions that need to be met for this function to be deprecated
| await reserve.addExchangeSpender(exchangeAddress) | ||
| await reserve.addExchangeSpender(accounts[1]) | ||
| const spenders = await reserve.getExchangeSpenders() | ||
| assert.equal(spenders.length, 2) |
There was a problem hiding this comment.
You should be able to check lists with the same contents in a single assert statement.
| }) | ||
|
|
||
| it('only allows owner', async () => { | ||
| await assertRevert(reserve.removeExchangeSpender(nonOwner, 0, { from: nonOwner })) |
There was a problem hiding this comment.
Should the first argument be exchangeSpenderAddress?
| }) | ||
| }) | ||
|
|
||
| it('has the right list of exchange spenders', async () => { |
|
Can you flesh out the PR description a bit more? |
Description
Allows the reserve to allow multiple exchanges to pull assets without a limit.
Added governable variables:
isExchangeSpenderOther changes
some more testing
Tested
Unit test added.
Related issues
Backwards compatibility
During the migrations, an exchange needs to be set.