Skip to content
This repository has been archived by the owner on Jul 9, 2021. It is now read-only.

Return array of FillResults in batch fill methods #1834

Merged
merged 7 commits into from
May 30, 2019

Conversation

abandeali1
Copy link
Member

@abandeali1 abandeali1 commented May 26, 2019

Description

  • Return an array of FillResults for the batch fill variants
  • Adds return value tests for all wrapper functions
  • Removes formatters from the test-utils package and no longer passes in any null assetData fields into any function calls. Note that we had a task to remove the calldata optimizations in matchOrders and the marketFill functions. For example, in matchOrders we have this rather than checking equality of the assetData fields:
rightOrder.makerAssetData = leftOrder.takerAssetData;
rightOrder.takerAssetData = leftOrder.makerAssetData;

I decided to leave these as is but clarify things a bit more in the comments, since this is actually cheaper than checking equality and still plays nicely with deduped calldata.

Testing instructions

Types of changes

Checklist:

  • Prefix PR title with [WIP] if necessary.
  • Add tests to cover changes as needed.
  • Update documentation as needed.
  • Add new entries to the relevant CHANGELOG.jsons.

@coveralls
Copy link

coveralls commented May 26, 2019

Coverage Status

Coverage remained the same at 79.778% when pulling e70b7f3 on feat/3.0/batchFillReturnValues into ba70c11 on 3.0.

@abandeali1 abandeali1 changed the title Feat/3.0/batch fill return values Return array of FillResults in batch fill methods May 26, 2019
@abandeali1 abandeali1 changed the base branch from 3.0 to feature/contracts/3.0/arbitrary-fees May 26, 2019 18:33
Copy link
Contributor

@merklejerk merklejerk left a comment

Choose a reason for hiding this comment

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

Nice job!

Copy link
Contributor

@hysz hysz left a comment

Choose a reason for hiding this comment

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

+1 looks good!

@dorothy-zbornak dorothy-zbornak force-pushed the feat/3.0/batchFillReturnValues branch from 0cc518c to fc5393e Compare May 30, 2019 21:11
@dorothy-zbornak dorothy-zbornak changed the base branch from feature/contracts/3.0/arbitrary-fees to 3.0 May 30, 2019 21:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants