CENT-631 - No way to decrement an allowance without reconfiguring minter #249
Conversation
|
tests |
contracts/minting/MintController.sol
Outdated
| * transaction to a minter and not worry about it being used to undo a removeMinter() | ||
| * transaction. | ||
| */ | ||
| function decrementMinterAllowance(uint256 allowanceDecrement) onlyController public returns (bool) { |
There was a problem hiding this comment.
please use solidity convention for input _allowanceDecrement
contracts/minting/MintController.sol
Outdated
| * transaction to a minter and not worry about it being used to undo a removeMinter() | ||
| * transaction. | ||
| */ | ||
| function decrementMinterAllowance(uint256 allowanceDecrement) onlyController public returns (bool) { |
There was a problem hiding this comment.
Please add require check for _allowanceDecrement must be greater than 0
There was a problem hiding this comment.
Replace all expectRevert with expectError. USDC unit tests do not have error messages in require/revert statements, so we use expectRevert. We added error messages to MINT contracts, so these unit tests can use expectError. The only exception is when the OpenZeppelin SafeMath library throws an error - in this case we expectJump. I pointed out a few place, but you need to go through and double-check.
Regarding newBigNumber vs new BigNumber, unit tests will fail until you make the change.
|
|
||
| uint256 currentAllowance = minterManager.minterAllowance(minter); | ||
| uint256 newAllowance = currentAllowance.sub(allowanceDecrement); | ||
|
|
There was a problem hiding this comment.
If the currentAllowance is less than the allowanceDecrement, the contract should set the allowance to 0. Issuers are going to have pre-signed incrementMinterAllowance and decrementMinterAllowance transactions. They need a way to drive the allowance down to 0 regardless of the current allowance value. The sub() function throws if allowanceDecrement>currentAllowance.
We can use the OpenZeppelin Math library:
uint256 newAllowance = max(currentAllowance, allowanceDecrement).sub(allowanceDecrement)
There was a problem hiding this comment.
If we're going with the "set to 0 rather than throw" approach, we should make sure _allowanceDecrement in the event accurately reflects how much was actually decremented - right now we are just reporting what we tried to decrement, but the actual amount decremented can be less if the new allowance is 0.
Also, we should rename the event MinterAllowanceDecremented and that parameter to amountDecremented. (All the other events in these new contracts are in past tense except IncrementAllowance, which I think should be also changed to past tense for consistency - normally it doesn't matter, but in this case it clarifies the behavior.)
There was a problem hiding this comment.
Yeah, you're right. The event is now firing the actual decremented amount
| expectedTokenState.push( | ||
| { 'variable': 'isAccountMinter.minterAccount', 'expectedValue': true }, | ||
| { 'variable': 'minterAllowance.minterAccount', 'expectedValue': new BigNumber(amount) } | ||
| ); |
There was a problem hiding this comment.
Use newBigNumber(amount) instead of new BigNumber(amount) to create a bn. Unlike the BigNumber library, the bn constructor takes as input only hex/binary. The newBigNumber is a wrapper function that takes decimal input and returns a bn.
| expectedTokenState.push( | ||
| { 'variable': 'isAccountMinter.minterAccount', 'expectedValue': true }, | ||
| { 'variable': 'minterAllowance.minterAccount', 'expectedValue': new BigNumber(amount) }, | ||
| ); |
There was a problem hiding this comment.
use newBigNumber(amount)
|
|
||
| it('only controller decrements allowance', async function () { | ||
| await expectRevert(mintController.decrementMinterAllowance(0, {from: Accounts.controller1Account})); | ||
| }); |
There was a problem hiding this comment.
use expectError to check for the correct error message.
|
|
||
| // decrement minter allowance | ||
| await expectRevert(mintController.decrementMinterAllowance(amount, {from: Accounts.controller1Account})); | ||
| }); |
There was a problem hiding this comment.
use expectError instead of expectRevert
contracts/minting/MintController.sol
Outdated
| function decrementMinterAllowance( | ||
| uint256 _allowanceDecrement | ||
| ) | ||
| public |
There was a problem hiding this comment.
Minor - lines 153–155 (public through returns (bool)) should be indented 4 spaces
No description provided.