Add natural unit restrictions for proposed Set. Create minimumBid amo… #207

bweick · 2018-09-18T17:44:54Z

…unt. Redo token flow math to use max component Set Token natural unit as base amount instead of 10**18. Figure out how many Sets to issue in settlement and recalcualte unitShares.

coveralls · 2018-09-18T17:50:44Z

Pull Request Test Coverage Report for Build 1895

64 of 64 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.007%) to 99.858%

Totals
Change from base Build 1881:	0.007%
Covered Lines:	508
Relevant Lines:	508

💛 - Coveralls

felix2feng · 2018-09-22T04:40:35Z

contracts/core/RebalancingSetToken.sol


 pragma solidity 0.4.24;

+import { AddressArrayUtils } from "../external/cryptofin/AddressArrayUtils.sol";


FYI - If you rebase with master, this import now uses the CryptoFin import.

contracts/core/RebalancingSetToken.sol

felix2feng · 2018-09-22T04:42:06Z

contracts/core/RebalancingSetToken.sol

     *
     *
-     * @param _rebalancingSet               The Set to rebalance into
+     * @param _nextSet               The Set to rebalance into


Knit: fix comment spacing

contracts/core/RebalancingSetToken.sol

felix2feng · 2018-09-22T04:49:29Z

contracts/core/RebalancingSetToken.sol


        for (uint256 i=0; i < combinedTokenArray.length; i++) {
-            uint256 rebalanceUnit = combinedRebalanceUnits[i];
+            uint256 nextUnit = combinedNextUnits[i];


Upon first glance, nextUnit seems like a confusing name. To be more discrete, maybe use something like nextSetUnit.

felix2feng · 2018-09-22T04:49:59Z

contracts/core/RebalancingSetToken.sol

+     * @return  combinedNextUnits
     */
-    function getCombinedRebalanceUnits()
+    function getCombinedNextUnits()


Maybe getCombinedNextSetUnits

felix2feng · 2018-09-22T04:55:02Z

contracts/core/RebalancingSetToken.sol

+     * @return uint256              Amount of nextSets to issue
+     * @return uint256              New unitShares for the rebalancingSetToken
+     */
+    function getIssueAmount()


We can name this better. It is currently too general and doesn't convey the work done. Perhaps something like calculateNextSetIssueQuantity.

felix2feng · 2018-09-23T21:08:25Z

contracts/core/RebalancingSetToken.sol

+        uint256 currentNaturalUnit = ISetToken(currentSet).naturalUnit();
+        uint256 rebalancingNaturalUnit = ISetToken(_nextSet).naturalUnit();
+        require(
+            Math.max256(currentNaturalUnit, rebalancingNaturalUnit) %


If we're going with nextSet, then the name of rebalancingNaturalUnit should be nextSetNaturalUnit or nextNaturalUnit

Ah yes I agree...missed that one

contracts/core/RebalancingSetToken.sol

felix2feng · 2018-09-23T21:13:17Z

contracts/core/RebalancingSetToken.sol

+            issueAmount
+        );
+
+        // Ensure allowance to transfer sets to Vault


Don't need comment.

Don't see the reason to be removing comments. They're free in my mind.

If the code is clear enough, we don't need explanatory comments (which is a good thing). I'm not opinionated about this one. So up to you.

I think above you talked about maintaining consistency and a long time ago we agreed on comments on every line and not assuming how obvious it is.

That being said, the comments could have a bit more color. In the case above, it could be, "Creating pointer to Core to Issue next set and Deposit into vault". This one could be: "Ensure transfer proxy has enough spender allowance to move issued nextSet to vault"

felix2feng · 2018-09-23T21:14:18Z

contracts/core/RebalancingSetToken.sol

+        // Create ICore object
+        ICore core = ICore(ISetFactory(factory).core());
+
+        // Issue nextSet and deposit in Vault


This step doesn't deposit into the vault does it?

contracts/core/RebalancingSetToken.sol

asoong · 2018-09-26T15:16:25Z

contracts/core/RebalancingSetToken.sol

+            issueAmount
+        );
+
+        // Ensure allowance to transfer sets to Vault


I think above you talked about maintaining consistency and a long time ago we agreed on comments on every line and not assuming how obvious it is.

That being said, the comments could have a bit more color. In the case above, it could be, "Creating pointer to Core to Issue next set and Deposit into vault". This one could be: "Ensure transfer proxy has enough spender allowance to move issued nextSet to vault"

contracts/core/RebalancingSetToken.sol

asoong · 2018-09-26T15:32:42Z

contracts/core/RebalancingSetToken.sol

        // Create interfaces for interacting with sets
        ISetToken currentSetInterface = ISetToken(currentSet);
-        ISetToken rebalancingSetInterface = ISetToken(rebalancingSet);
+        ISetToken nextSetInterface = ISetToken(nextSet);


Omit this interface suffix

asoong · 2018-09-26T15:36:18Z

contracts/core/RebalancingSetToken.sol

+                this
+            );
+
+            // Calculate amount of Sets that can be issued from those components, if less than amount for other


Examples of great comments

asoong · 2018-09-26T15:37:06Z

contracts/core/RebalancingSetToken.sol

+        uint256 naturalUnitsOutstanding = totalSupply_.div(naturalUnit);
+
+        // Issue amount of Sets that is closest multiple of nextNaturalUnit to the maxIssueAmount
+        uint256 issueAmount = maxIssueAmount.div(nextNaturalUnit).mul(nextNaturalUnit);


Forgot, where does the dust that is not used to issue remain and who is the owner?

Remains in the vault under the rebalancingSetToken's "ownership" so could be used in a future rebalance.

who is ownership in this case? the manager? or they just belong to the reblanacing set token?

asoong · 2018-09-26T15:38:05Z

contracts/core/interfaces/ICore.sol

+     * Returns if valid set
     *
-     * @return  uint256       Natural unit of Set
+     * @return  bool      If valid set


Improve message here: "Returns whether set was...."

asoong · 2018-09-26T15:39:44Z

test/core/extensions/coreIssuanceOrder.spec.ts

    it('transfers the full maker token amount from the maker', async () => {
      const existingBalance = await makerToken.balanceOf.callAsync(issuanceOrderMaker);
-      await assertTokenBalance(makerToken, DEPLOYED_TOKEN_QUANTITY, issuanceOrderMaker);
+      await assertTokenBalanceAsync(makerToken, DEPLOYED_TOKEN_QUANTITY, issuanceOrderMaker);


Next time make these changes in a separate PR and rebase yours onto it

Ignoring spec files unrelated to rebalancing

asoong · 2018-09-26T15:40:14Z

test/core/extensions/coreRebalanceAuction.spec.ts

 const blockchain = new Blockchain(web3);


+


felix2feng · 2018-09-26T18:22:18Z

contracts/core/RebalancingSetToken.sol

-        require(ICore(ISetFactory(factory).core()).validSets(_rebalancingSet));
+        require(ICore(ISetFactory(factory).core()).validSets(_nextSet));
+
+        // Check that the propoosed set is a multiple of current set, or vice versa


Knit: Check that the proposed Set natural unit is a multiple of current Set natural unit

felix2feng · 2018-09-27T03:29:13Z

utils/RebalancingTokenWrapper.ts

+      // Determine minimum natural unit if not passed in
+      if (naturalUnits) {
+        naturalUnit = naturalUnits[idx];
+        minDec = 18 - naturalUnit.e;


What is .e?

Exponent of the BigNumber

felix2feng · 2018-09-27T03:30:06Z

utils/RebalancingTokenWrapper.ts


    const indexArray = _.times(tokenCount, Number);
    for (const index in indexArray) {
+      let minDec: number;


Let's favor being verbose and descriptive in variable names.

minimumDecimal vs minDec

felix2feng · 2018-09-27T03:30:34Z

utils/RebalancingTokenWrapper.ts

    for (const index in indexArray) {
+      let minDec: number;
      const idx = Number(index);
+      const decOne = await components[idx].decimals.callAsync();


Same here re: verbose

componentOneDecimal vs decOne

felix2feng · 2018-09-27T03:33:33Z

utils/RebalancingTokenWrapper.ts

      const setComponents = components.slice(idx, idx + 2);
      const setComponentAddresses = _.map(setComponents, token => token.address);
-      const setComponentUnits = _.map(setComponents, () => naturalUnit.mul(idx + 1)); // Multiple of naturalUnit
+      const setComponentUnits: BigNumber[] =


Improve formatting / readability by putting braces on their own lines

felix2feng · 2018-09-27T03:35:24Z

utils/RebalancingTokenWrapper.ts

      const setComponentAddresses = _.map(setComponents, token => token.address);
-      const setComponentUnits = _.map(setComponents, () => naturalUnit.mul(idx + 1)); // Multiple of naturalUnit
+      const setComponentUnits: BigNumber[] =
+      [new BigNumber(10 ** (decOne.toNumber() - minDec)).mul(new BigNumber(idx + 1)),


Can we have simpler / intermediate calculations for component units?

…unt. Redo token flow math to use max component Set Token natural unit as base amount instead of 10**18. Figure out how many Sets to issue in settlement and recalcualte unitShares.

…ory then committed to storage. Added missing test for bid quantities that are not multiples of minimum bid.

asoong

Some comments to address in future PRs, can you also rename RebalancingTokenWrapper.ts to rebalancingWrapper.ts

asoong · 2018-09-27T21:15:02Z

contracts/core/RebalancingSetToken.sol

+                    nextUnit.mul(auctionPriceDivisor).sub(currentUnit.mul(priceNumerator))
+                ).div(priceNumerator);
+
+                // Set outflow amount to 0 for component i


In subsequent PRs, improve comments here. "This bid price requires component i, so set outflow amount to 0". Sometimes we're too literal with our comments.

asoong · 2018-09-27T21:15:10Z

contracts/core/RebalancingSetToken.sol

+                    currentUnit.mul(priceNumerator).sub(nextUnit.mul(auctionPriceDivisor))
+                ).div(priceNumerator);
+
+                // Set inflow amount to 0 for component i


asoong · 2018-09-27T21:16:07Z

contracts/core/RebalancingSetToken.sol

+        uint256 naturalUnitsOutstanding = totalSupply_.div(naturalUnit);
+
+        // Issue amount of Sets that is closest multiple of nextNaturalUnit to the maxIssueAmount
+        uint256 issueAmount = maxIssueAmount.div(nextNaturalUnit).mul(nextNaturalUnit);


who is ownership in this case? the manager? or they just belong to the reblanacing set token?

asoong · 2018-09-27T21:18:19Z

test/core/rebalancingSetToken.spec.ts

      });

-      describe('but the proposed rebalancingSet is not approved by Core', async () => {
+      describe('but the proposed nextSet is not approved by Core', async () => {


but? update to when in subsequent pr

I would look at it in the context of the whole statement being printed out "when propose is called from the Default state"..."but"

asoong · 2018-09-27T21:19:04Z

test/core/rebalancingSetToken.spec.ts

+
+      describe("but the new proposed set's natural unit is not a multiple of the current set", async () => {
+        before(async () => {
+          naturalUnits = [ether(.003), ether(.002), ether(.001)];


Add a comment to clarify these values. What is it being compared against that it is not a multiple of

felix2feng reviewed Sep 22, 2018

View reviewed changes

bweick force-pushed the brian/complete_rebalance_logic branch from 02ef79f to aae751d Compare September 23, 2018 17:12

felix2feng reviewed Sep 23, 2018

View reviewed changes

asoong reviewed Sep 26, 2018

View reviewed changes

asoong force-pushed the brian/complete_rebalance_logic branch from 6b59da2 to 3fc00d4 Compare September 26, 2018 19:33

felix2feng reviewed Sep 26, 2018

View reviewed changes

felix2feng reviewed Sep 27, 2018

View reviewed changes

bweick and others added 3 commits September 27, 2018 12:52

Add natural unit restrictions for proposed Set. Create minimumBid amo…

5298ce3

…unt. Redo token flow math to use max component Set Token natural unit as base amount instead of 10**18. Figure out how many Sets to issue in settlement and recalcualte unitShares.

Update gas limits

3e39350

Added comments. Changed combinedUnitArray formation to be done in mem…

ec1c502

…ory then committed to storage. Added missing test for bid quantities that are not multiples of minimum bid.

bweick force-pushed the brian/complete_rebalance_logic branch from 241cef9 to ec1c502 Compare September 27, 2018 19:52

asoong approved these changes Sep 27, 2018

View reviewed changes

Updated some comments.

6f3e52f

bweick merged commit c4d68b8 into master Sep 27, 2018

bweick deleted the brian/complete_rebalance_logic branch September 27, 2018 22:42


		pragma solidity 0.4.24;

		import { AddressArrayUtils } from "../external/cryptofin/AddressArrayUtils.sol";

Add natural unit restrictions for proposed Set. Create minimumBid amo… #207

Add natural unit restrictions for proposed Set. Create minimumBid amo… #207

Uh oh!

Conversation

bweick commented Sep 18, 2018

Uh oh!

coveralls commented Sep 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1895

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Sep 18, 2018 •

edited

Loading