contracts: Convert Troves to ERC721 NFTs #81
Conversation
|
Yikes, the merge conflicts with #76 are pretty substantial already 😬 I recommend we do something about it ASAP. |
bingen
force-pushed
the
trove_ERC721
branch
3 times, most recently
from
d51d023
to
0381401
Compare
| * Otherwise, only the address in this mapping (and the trove owner) will be allowed. | ||
| * To restrict this permission to no one, trove owner should be set in this mapping. | ||
| */ | ||
| mapping (uint256 => address) public TroveAddManagers; |
There was a problem hiding this comment.
So we're only having one "AddManager" and one "RemoveManager" per NFT right? I guess we could also do a whitelist of multiple, but I prefer the simplicity of only one
There was a problem hiding this comment.
Yes, this is what I understood when we discussed during a dev call. I thought a little bit about this. As you mention, I like the simplicity and gas efficiency of doing it this way. And more complex solutions can be built on top. For instance, setting as manager a 1/n multisig would allow n addresses to effectively be the manager. But I don’t expect that to be most common use case.
| } | ||
|
|
||
| function adjustTroveInterestRate(uint _newAnnualInterestRate, address _upperHint, address _lowerHint) external { | ||
| function adjustTroveInterestRate(uint256 _troveId, uint _newAnnualInterestRate, uint256 _upperHint, uint256 _lowerHint) external { |
There was a problem hiding this comment.
Hmm, where's the access control now for this? Previously it was implicit in _requireTroveIsActive right? So I guess now we should explicitly check that the caller owns the Trove ID (and we're missing a test that checks for the revert)
There was a problem hiding this comment.
Good catch! Fixing and adding test.
contracts/src/TroveManager.sol
Outdated
| // Save the uint256 of the Trove preceding the current one, before potentially modifying the list | ||
| uint256 nextUserToCheck = contractsCache.sortedTroves.getPrev(currentTroveId); | ||
| // TODO: check ICR? | ||
| //getCurrentICR(currentTroveId, _price) < MCR |
There was a problem hiding this comment.
Right, the loop could sometimes find Troves with ICR < MCR, which didn't happen in v1 since we started at >= 110%. I guess we can just skip them if and when the loop finds them.
I suppose then this system will still has the property that redemptions can reduce the TCR when there is underwater debt (Troves with CR < MCR). Though if we do remove Recovery Mode the attack potential should be even smaller than v1.
Maybe we'd only skip at ICR < 100%, which would mean that redemptions still always improve the CR of Troves they hit, and it makes it less likely that redemptions reduce the TCR (since some of the underwater debt would be redeemed from)
There was a problem hiding this comment.
Good point, I’ll do that.
Closes #12
Closes #64