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

Optimize 0x wrapper, reduce unnecessary slices #155

Merged
merged 4 commits into from Aug 9, 2018

Conversation

asoong
Copy link
Contributor

@asoong asoong commented Aug 9, 2018

Clarify 0x order library parsing of order data, reduce number of slices. Saving around 40k gas in the process.

@asoong asoong requested a review from a team August 9, 2018 07:59
@asoong asoong force-pushed the alex/optimize_zero_ex_wrapper branch from 43dc288 to 3837afe Compare August 9, 2018 08:08
@coveralls
Copy link

coveralls commented Aug 9, 2018

Pull Request Test Coverage Report for Build 1144

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

Totals Coverage Status
Change from base Build 1118: 0.0%
Covered Lines: 321
Relevant Lines: 321

💛 - Coveralls

@@ -72,42 +73,65 @@ contract ZeroExExchangeWrapper {
/* ============ Public Functions ============ */

/**
* IExchangeWrapper interface delegate method.
* IExchange interface delegate method.
Copy link
Contributor

Choose a reason for hiding this comment

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

I made some changes to call everything IExchangeWrapper vs IExchange. Thought it was clearer

Copy link
Contributor Author

@asoong asoong Aug 9, 2018

Choose a reason for hiding this comment

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

+1, this file was heavily contested in the rebase so I just went with mine. Will make these changes.

*/
function exchange(
address _taker,
address _,
Copy link
Contributor

Choose a reason for hiding this comment

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

is _ a good name? Could it ever get conflated with _ in a modifier?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pretty standard nomenclature for unused variables, just to be clear now. It will get used when we work in relayer fees.

* | expirationUnixTimeStampSec | 256 |
* | salt | 288 |
* | makerAssetData | 320 |
* | takerAssetData | 320 |
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this accurate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1, updated

bytes memory orderBody = _ordersData.slice(
_offset,
_offset.add(orderLength)
uint256 takerAssetStart = _header.makerAssetDataLength.add(320);
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

@asoong asoong merged commit 457bf5b into master Aug 9, 2018
@asoong asoong deleted the alex/optimize_zero_ex_wrapper branch August 9, 2018 18:19
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.

None yet

3 participants