Exchange Issue into RB Set Using Eth #343
Conversation
Pull Request Test Coverage Report for Build 3654
💛 - Coveralls |
felix2feng
changed the title
| uint256 rbSetNaturalUnit = ISetToken(_rebalancingSetAddress).naturalUnit(); | ||
|
|
||
| // Ensure that the base Set quantity is a multiple of the rebalancing Set natural unit | ||
| uint256 rbSetNormalizedBaseSetQuantity = _baseSetIssueQuantity.div(rbSetNaturalUnit).mul(rbSetNaturalUnit); |
There was a problem hiding this comment.
I'm unsure what this line of code is supposed to do. Is this flipped with the line below? Wouldn't you want to see how many rebalancing set tokens you can issue and then make sure it's a multiple of the rebalancing set?
There was a problem hiding this comment.
+1 Think its flipped
.solcover.js
Outdated
| 'lib/AddressArrayUtils.sol', | ||
| 'mocks', | ||
| 'external', | ||
| 'supplementary/PayableExchangeIssue.sol' // Transfer functions cannot be properly tested |
There was a problem hiding this comment.
Can you link to the github issue here?
| */ | ||
| interface IExchangeIssueModule { | ||
|
|
||
|
|
| library ExchangeIssueLibrary { | ||
| // ============ Structs ============ | ||
|
|
||
| struct ExchangeIssue { |
There was a problem hiding this comment.
Can we rename this? ExchangeIssueParams? ExchangeIssueRequest?
| { | ||
| // Commit the address and instance of Core to state variables | ||
| core = _core; | ||
| coreInstance = ICore(_core); |
There was a problem hiding this comment.
Why do we store both of these again?
There was a problem hiding this comment.
Slightly more gas efficient and cleans up multiple instantiations of instances
| private | ||
| { | ||
| // Return any excess base Set to the user | ||
| uint256 leftoverBaseSet = ERC20Wrapper.balanceOf( |
There was a problem hiding this comment.
This is a weird edge case, will the user possibly be expecting balance of a static set? I think we should give these to the rebalancing set @bweick. That way, when the rebalance starts, all of it gets redeemed and the excess can be converted into shares of the new set? Not sure if that messes with the supply, but we do calculate the "max issue amount" at the end of the rebalance and these components could contribute to it.
| private | ||
| returns (uint256) | ||
| { | ||
| uint256[] memory rbSetUnits = ISetToken(_rebalancingSetAddress).getUnits(); |
There was a problem hiding this comment.
Can we have a separate getter for rebalancing set token that clarifies what this is instead of doing units[0]? Add it to the IRebalancingSetToken maybe?
There was a problem hiding this comment.
Hmmm good call. We have something called Unit shares already. I'll use that.
| // Ensure that the base Set quantity is a multiple of the rebalancing Set natural unit | ||
| uint256 rbSetIssueQuantity = possibleIssuableRBSetQuantity.div(rbSetNaturalUnit).mul(rbSetNaturalUnit); | ||
|
|
||
|
|
| private | ||
| returns (uint256) | ||
| { | ||
| uint256[] memory rbSetUnits = ISetToken(_rebalancingSetAddress).getUnits(); |
There was a problem hiding this comment.
Why shorthand this? rbSetUnits is not clear. Just call it unitShares maybe?
felix2feng
left a comment
felix2feng
left a comment
There was a problem hiding this comment.
Addressed all except for the Base Set refund. In theory, our application / backend should never produce more base Sets than needed (it would be a bug).
| { | ||
| // Commit the address and instance of Core to state variables | ||
| core = _core; | ||
| coreInstance = ICore(_core); |
There was a problem hiding this comment.
Slightly more gas efficient and cleans up multiple instantiations of instances
| private | ||
| returns (uint256) | ||
| { | ||
| uint256[] memory rbSetUnits = ISetToken(_rebalancingSetAddress).getUnits(); |
There was a problem hiding this comment.
Hmmm good call. We have something called Unit shares already. I'll use that.
| private | ||
| returns (uint256) | ||
| { | ||
| uint256[] memory rbSetUnits = ISetToken(_rebalancingSetAddress).getUnits(); |
ChangeLog: