[WIP] Brian/auction set up logic #181

bweick · 2018-08-21T17:22:38Z

No description provided.

coveralls · 2018-08-22T23:21:49Z

Pull Request Test Coverage Report for Build 1535

27 of 27 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 1514:	0.0%
Covered Lines:	421
Relevant Lines:	421

💛 - Coveralls

felix2feng · 2018-08-22T23:25:13Z

contracts/core/RebalancingSetToken.sol

+        uint256[] memory currentSetUnits = currentSetInterface.getUnits();
+        uint256[] memory rebalancingSetUnits = rebalancingSetInterface.getUnits();
+
+        for (uint16 i=0; i < combinedTokenArray.length; i++) {


Let's use uint256 vs uint16 - since there are no gas savings

felix2feng · 2018-08-22T23:26:32Z

contracts/core/RebalancingSetToken.sol

+     * @param _unit             The units of the component token
+     * @param _naturalUnit      The natural unit of the Set token
+     */
+    function computeUnits(


Do we have one of these functions elsewhere? If so, maybe we can avoid duplication by using a lib

We have something similar in Core but don't see a need to have us make a call to it in order to calculate this.

felix2feng · 2018-08-22T23:28:40Z

test/core/rebalancingSetToken.spec.ts

 const RebalancingSetToken = artifacts.require('RebalancingSetToken');
+const Core = artifacts.require('CoreMock');
+
+// import { injectInTruffle } from 'sol-trace-set';


Remove if not needed?

felix2feng · 2018-08-22T23:29:08Z

test/core/rebalancingSetToken.spec.ts

 ChaiSetup.configure();
 const { expect } = chai;
 const RebalancingSetToken = artifacts.require('RebalancingSetToken');
+const Core = artifacts.require('CoreMock');


Make naming consistent? Core and CoreMock. Maybe go with CoreMock so that future we don't trip over this.

felix2feng · 2018-08-22T23:30:05Z

test/core/rebalancingSetToken.spec.ts

  let rebalancingSetToken: RebalancingSetTokenContract;
  let components: StandardTokenMockContract[] = [];
+
+  let core: CoreMockContract;


wonder if we should rename this all to coreMock to be clear

I don't see a reason to. We're only using CoreMock in this test suite...but it's still what we're using as Core.

+1, let's use coreMock here when it's coreMock

felix2feng · 2018-08-22T23:30:19Z

test/core/rebalancingSetToken.spec.ts


  before(async () => {
    await blockchain.saveSnapshotAsync();
+    ABIDecoder.addABI(Core.abi);


CoreMock?

felix2feng · 2018-08-22T23:30:52Z

test/core/rebalancingSetToken.spec.ts

-        blockchain.increaseTimeAsync(timeFastForward);
-        await rebalancingSetToken.propose.sendTransactionAsync(
-          newRebalancingToken,
+        await blockchain.increaseTimeAsync(timeFastForward);


felix2feng · 2018-08-22T23:31:30Z

test/core/rebalancingSetToken.spec.ts

+  });
+
+  describe('#mint called from Core', async () => {
+    let rebalancingToken: RebalancingSetTokenContract;


As we discussed - a lot of this setup could be moved into a wrapper.

Yep another PR methinks

Yes and no, a lot of this stuff is unnecessary such as declaring a new rebalancingToken and setName, setSymbol (defined in coreWrapper already) that I noticed. I cleaned this up in my PR that should be on master. Makes following along with specs easier

felix2feng · 2018-08-22T23:34:20Z

@asoong Could you help do a review as well?

…code to redeem currentSets when rebalance called. Waiting on RebalancingTokenFactory to complete tests.

…nt256 on rebalancing set token.

asoong · 2018-08-23T00:36:25Z

contracts/core/RebalancingSetToken.sol

+                    computeUnits(currentSetUnits[indexCurrent], currentSetNaturalUnit)
+                );
+            } else {
+                combinedCurrentUnits.push(uint256(0));


interesting, do you have to cast this here? uint256(0)?

Prob not but feel like we've fallen on the standard of explicitly stating what all the uints are

asoong · 2018-08-23T00:41:08Z

test/core/rebalancingSetToken.spec.ts

    let subjectCaller: Address;

-    let newRebalancingToken: Address;
+    let currentSetToken: SetTokenContract;


e.g. this is not being referenced elsewhere in this spec, don't need to define a variable

asoong · 2018-08-23T00:41:14Z

test/core/rebalancingSetToken.spec.ts

-    let newRebalancingToken: Address;
+    let currentSetToken: SetTokenContract;
+
+    const naturalUnit: BigNumber = ether(2);


asoong · 2018-08-23T00:46:29Z

test/core/rebalancingSetToken.spec.ts

  });

-  describe('#mint', async () => {
+  describe('#mint called directly', async () => {


The whole point of these specs is to make sure the require(msg.sender == ISetFactory(factory).core()); triggers correct? I say keep this section short and move the other test cases into #mint called from Core to be clear. Right now you have:

#mint called directly:

it updates the balances of the user correctly

it updates the totalSupply_ correctly

it emits a Transfer log denoting a minting

when the caller is not Core

#mint called from Core

it updates the balances of the user correctly

when mint is called from Rebalance state
..?

New:
#mint called directly:
when the caller is not Core

#mint called from Core
it updates the balances of the user correctly
it updates the totalSupply_ correctly
it emits a Transfer log denoting a minting
when mint is called from Rebalance state

Agree mostly do you think it makes sense to keep one "happy" path case in the "direct" case so that we aren't accidentally missing something there?

asoong · 2018-08-23T00:48:58Z

test/core/rebalancingSetToken.spec.ts

+    let subjectQuantity: BigNumber;
+    let subjectCaller: Address;
+
+    let currentSetToken: SetTokenContract;


Remove anything not referenced later in this test case

asoong · 2018-08-23T00:49:21Z

test/core/rebalancingSetToken.spec.ts

+    let currentSetToken: SetTokenContract;
+    let newRebalancingSetToken: SetTokenContract;
+
+    const naturalUnit: BigNumber = ether(2);


asoong · 2018-08-23T00:56:18Z

test/core/rebalancingSetToken.spec.ts

+      const manager = managerAccount;
+      const initialSet = currentSetToken.address;
+      const initialUnitShares = new BigNumber(1);
+      const rebalanceInterval = new BigNumber(90000);


Ideally a constant for these numbers

asoong · 2018-08-23T00:59:56Z

test/core/rebalancingSetToken.spec.ts

+      it('creates the correct combinedCurrentUnits', async () => {
+        await subject();
+
+        const expectedCombinedCurrentUnits = [ether(2), ether(2), ether(0)];


where do these come from?

asoong · 2018-08-23T01:01:39Z

test/core/rebalancingSetToken.spec.ts

        const timeFastForward = 100000;

-        blockchain.increaseTimeAsync(timeFastForward);
+        await blockchain.increaseTimeAsync(timeFastForward);


Would really love for this whole beforeEach to be one wrapper function that encompasses all the state transitions

asoong · 2018-08-23T01:03:18Z

test/core/rebalancingSetToken.spec.ts

+      components = await erc20Wrapper.deployTokensAsync(3, deployerAccount);
+      await erc20Wrapper.approveTransfersAsync(components, transferProxy.address);
+
+      const currentComponentAddresses = _.map(components.slice(0, 2), token => token.address);


Define a constant for this slice

asoong · 2018-08-23T01:03:48Z

test/core/rebalancingSetToken.spec.ts

+        naturalUnit,
+      );
+
+      const newComponentAddresses = _.map(components.slice(1, 3), token => token.address);


asoong · 2018-08-23T01:05:41Z

test/core/rebalancingSetToken.spec.ts

-      factory = await coreWrapper.deploySetTokenFactoryAsync(coreAccount);
+      await erc20Wrapper.approveTransfersAsync(components, transferProxy.address);
+
+      const currentComponentAddresses = _.map(components.slice(0, 2), token => token.address);


Why do you need to slice here?

asoong · 2018-08-23T01:07:15Z

Doing some cleanup now will help you in the next phase. @felix2feng In the future, let's keep the PR smaller. Maybe start with CoreMock#mint specs first before moving on to burn.

…kenWrapper.

…erate expected arrays during propose.

felix2feng

This is a SIGNIFICANT improvement. I'm deferring to @asoong for thumbs up

felix2feng · 2018-08-23T20:24:16Z

utils/RebalancingTokenWrapper.ts

+    this._blockchain = blockchain;
+  }
+
+  public async createSetTokens(


This doesn't seem rebalancing specific. Should this be in CoreWrapper instead?

If we wanna move it at some point we can. Just feels like the only functionality we need multiple SetTokens for rn is rebalancing.

felix2feng · 2018-08-23T20:25:05Z

utils/RebalancingTokenWrapper.ts

+    );
+  }
+
+  public async constructCombinedUnitArray(


Could we add a javadoc on what this does?

felix2feng · 2018-08-23T20:27:06Z

test/core/rebalancingSetToken.spec.ts

+    let newRebalancingSetToken: SetTokenContract;
+
+    beforeEach(async () => {
+      const setTokens = await rebalancingTokenWrapper.createSetTokens(coreMock, factory.address, transferProxy.address);


You could combine this and the next two lines in one line

const [ currentSetToken, newRebalancingSetToken ] = await rebalancingTokenWrapper.createSetTokens...

Different scoping on them though

felix2feng · 2018-08-23T20:27:30Z

test/core/rebalancingSetToken.spec.ts

+        proposalPeriod
+      );
+
+


extra new line

felix2feng · 2018-08-23T20:29:18Z

test/core/rebalancingSetToken.spec.ts

-      factory = await coreWrapper.deploySetTokenFactoryAsync(coreAccount);
+      await erc20Wrapper.approveTransfersAsync(components, transferProxy.address);
+
+      const currentComponentAddresses = _.map(components, token => token.address);


Could we put the map function in a util or something? It's a bit jarring to see this low level stuff in a test

Those tests are gonna be deleted anyway but I also don't think it's worthwhile to create a full other util for one line of code.

Sorry not gonna be deleted was looking at wrong test suite but still don't think its worth creating its own util... _.map is the util.

asoong

LGTM. I left comments on some more improvements you can address as part of this or the next PR. Also consider moving propose and bid specs into its own file

asoong · 2018-08-23T20:19:01Z

utils/RebalancingTokenWrapper.ts

+    );
+  }
+
+  public async defaultTransitionToPropose(


Why are these prefixed to default?

default -> propose state

asoong · 2018-08-23T20:19:47Z

utils/RebalancingTokenWrapper.ts

+    this._blockchain = blockchain;
+  }
+
+  public async createSetTokens(


suffix methods Async

asoong · 2018-08-23T20:20:29Z

utils/RebalancingTokenWrapper.ts

+    const components = await this._erc20Wrapper.deployTokensAsync(3, this._tokenOwnerAddress);
+    await this._erc20Wrapper.approveTransfersAsync(components, transferProxy);
+
+    const set1Components = components.slice(0, 2);


nit: don't use numbers. firstSetComponents

asoong · 2018-08-23T20:22:26Z

utils/constants.ts

 export const ZRX_TOKEN_ADDRESS = '0x871dd7c2b4b25e1aa18728e9d5f2af4c4e431f5c';

+// Rebalancing Constants
+export const DEFAULT_PERIOD_INTERVAL = new BigNumber(90000);


Helps to be even more realistic here for clarity. Your contracts specify day cutoffs. Create constants for maybe ONE_DAY_IN_SECONDS and your test case for invalid PERIOD_INTERVAL can be ONE_DAY_IN_SECONDS.sub(1)

It makes your usage more readable: const proposalPeriod = ONE_DAY_IN_SECONDS; vs.
const proposalPeriod = DEFAULT_PERIOD_INTERVAL;

Expired

… Renamed some variables.

felix2feng force-pushed the brian/auction_set_up_logic branch from d8c4754 to 97cbbe0 Compare August 22, 2018 18:12

bweick requested a review from a team August 22, 2018 23:24

felix2feng previously requested changes Aug 22, 2018

View reviewed changes

bweick and others added 7 commits August 22, 2018 16:54

Added logic to create token and unit arrays for bidding calcs. Added …

6fcfb86

…code to redeem currentSets when rebalance called. Waiting on RebalancingTokenFactory to complete tests.

Integrating RebalancingTokenFactory to tests.

572281f

Pre-rebase commit (#185)

f34d327

Finish renaming

ed95a4c

Tests fixed using CoreMock.

f1ee0af

Removing unused var.

54a67e0

Minor renaming fixes in tests. Changed loop counter from uint16 -> ui…

fd37b35

…nt256 on rebalancing set token.

asoong reviewed Aug 23, 2018

View reviewed changes

Rebalancing tests refactor. Moved common functions into RebalancingTo…

0f73195

…kenWrapper.

bweick force-pushed the brian/auction_set_up_logic branch from 6b1f608 to 0f73195 Compare August 23, 2018 18:04

Added getters for arrays created during propose. Revised tests to gen…

27152ef

…erate expected arrays during propose.

felix2feng reviewed Aug 23, 2018

View reviewed changes

asoong approved these changes Aug 23, 2018

View reviewed changes

bweick added 2 commits August 23, 2018 14:47

Renamed constants and gave new values. Changed helper function names.…

49a1a2a

… Renamed some variables.

Small fix to make sure no block time error.

aa3933e

bweick merged commit b490857 into master Aug 23, 2018

bweick deleted the brian/auction_set_up_logic branch August 23, 2018 22:16

[WIP] Brian/auction set up logic #181

[WIP] Brian/auction set up logic #181

Uh oh!

Conversation

bweick commented Aug 21, 2018

Uh oh!

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

Uh oh!

Pull Request Test Coverage Report for Build 1535

💛 - Coveralls

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!

felix2feng commented Aug 22, 2018

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!

asoong commented Aug 23, 2018

Uh oh!

felix2feng left a comment

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 Aug 22, 2018 •

edited

Loading