Skip to content
This repository was archived by the owner on Jan 18, 2023. It is now read-only.

Conversation

bweick
Copy link
Contributor

@bweick bweick commented Jul 5, 2018

No description provided.

describe("when the set was not created through core", async () => {
beforeEach(async () => {
issuanceOrderParams = await generateFillOrderParameters(NULL_ADDRESS, signerAddress, signerAddress, componentAddresses[0])
issuanceOrderParams = await generateFillOrderParameters(NULL_ADDRESS, signerAddress, signerAddress, componentAddresses[0], relayerAddress);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the maker address you're passing in here also signerAddress?

});
});

describe("when the order quantity is not a multiple of the natural unit of the set", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In these specs where the setup is all common, can you help identify what is is you're toggling? I would put generateFillOrderParameters. In beforeEach on line 124, you're already doing this generation, and then you're regenerating for each of these test cases. I'd create a variable for each one and then just toggle the specific variable in each test case.

function validateOrder(
OrderLibrary.IssuanceOrder _order,
uint _fillQuantity
uint _executeQuantity
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference behind executeQuantity and fillQuantity? Are you just making it generic for this private function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Si Papi

const naturalUnit: BigNumber = ether(2);
let components: StandardTokenMockContract[] = [];
let componentUnits: BigNumber[];
let setToken: SetTokenContract;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's tighten these up, remove any variables here, also in cancelOrder for variables that don't need to be toggled.

naturalUnit, components, componentUnits, setToken

let issuanceOrderParams: any;

beforeEach(async () => {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: space

@bweick bweick force-pushed the brian/open_order_amount branch from c3297c1 to b0b6044 Compare July 6, 2018 01:36
…onciling fill and cancel amounts with open order size. Added some natural unit checks on fill and cancel quantities.
@bweick bweick force-pushed the brian/open_order_amount branch from b0b6044 to 0190c80 Compare July 6, 2018 01:38
@coveralls
Copy link

coveralls commented Jul 6, 2018

Pull Request Test Coverage Report for Build 448

  • 10 of 10 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 97.239%

Totals Coverage Status
Change from base Build 440: 0.1%
Covered Lines: 230
Relevant Lines: 238

💛 - Coveralls

@bweick bweick merged commit 3a9cfa1 into master Jul 6, 2018
@bweick bweick deleted the brian/open_order_amount branch July 6, 2018 01:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants