Conversation
address(this) | ||
); | ||
|
||
require( |
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.
@asoong Instead of that funky return baseSet in the payableExchangeIssue function, we could do something like a require where the input issue or rebalance quantity needs to work
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.
Yeah I like this better, with the tradeoff being issuance quantity granularity?
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.
+1, let's get some scenario tests for this up to better understand how this affects issuances, especially after rebalances
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.
LGTM
baseSetAddress, | ||
msg.sender, | ||
baseSetRedeemQuantity, | ||
0 |
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.
Don't we want to forward the toExclude
property up too? On setProtocol.js we can default it to 0
* @param _rebalancingSetAddress Address of the rebalancing Set to redeem | ||
* @param _redeemQuantity The Quantity of the rebalancing Set to redeem | ||
*/ | ||
function redeemRBSetIntoBaseComponents( |
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.
Still dislike the RB
abbreviation, prefer rebalancing spelled out
address(this) | ||
); | ||
|
||
require( |
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.
+1, let's get some scenario tests for this up to better understand how this affects issuances, especially after rebalances
baseSetComponent = await erc20Wrapper.deployTokenAsync(ownerAccount); | ||
await erc20Wrapper.approveTransferAsync(baseSetComponent, transferProxy.address); | ||
|
||
// Create the Set (1 component) |
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.
These tests work fine to verify basic domain logic, definitely need better examples with scenario tests
}); | ||
|
||
describe('#redeemRBSetIntoBaseComponents', async () => { | ||
const subjectCaller: Address = functionCaller; |
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've moved away from initializing anything when declaring variables, prefer all the values to be set together
let subjectRedeemQuantity: BigNumber; | ||
|
||
let baseSetIssueQuantity: BigNumber; | ||
const baseSetComponentUnit: BigNumber = new BigNumber(10 ** 10); |
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.
Same
2da801c
to
9979871
Compare
Pull Request Test Coverage Report for Build 3705
💛 - Coveralls |
Changelog
RedeemAndWithdrawTo
redeemRBSetIntoBaseComponents
function and module