This repository has been archived by the owner on Jul 9, 2021. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 465
update abi-gen with new method interfaces #2325
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
xianny
force-pushed
the
refactor/abi-gen-wrappers
branch
2 times, most recently
from
November 13, 2019 01:45
bcb5686
to
9d37c22
Compare
xianny
force-pushed
the
refactor/abi-gen-wrappers
branch
from
November 13, 2019 01:55
9d37c22
to
d58b770
Compare
xianny
force-pushed
the
refactor/abi-gen-wrappers
branch
from
November 13, 2019 12:26
d58b770
to
1ee22b3
Compare
…onorepo into refactor/abi-gen-wrappers
|
xianny
force-pushed
the
refactor/abi-gen-wrappers
branch
from
November 13, 2019 22:17
74ad8cf
to
85388fe
Compare
xianny
force-pushed
the
refactor/abi-gen-wrappers
branch
from
November 13, 2019 22:51
85388fe
to
c61ada8
Compare
xianny
requested review from
abandeali1,
BMillman19,
dekz,
fabioberger,
feuGeneA,
fragosti,
hysz and
steveklebanoff
as code owners
November 14, 2019 05:43
xianny
force-pushed
the
refactor/abi-gen-wrappers
branch
from
November 14, 2019 05:56
c4d62d4
to
2fce39b
Compare
xianny
force-pushed
the
refactor/abi-gen-wrappers
branch
from
November 14, 2019 06:24
2fce39b
to
ac6ab0b
Compare
dekz
approved these changes
Nov 14, 2019
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good, size reduction is great and ultimately I think the outcome is more readable.
One discrepancy in types
CHANGELOG
which may have already been published.
I think we should have a CHANGELOG for contract-wrappers
,abi-gen
and perhaps abi-gen-wrappers
, but I don't think many people use those directly. Do you think it's worth calling out the changes in any other packages?
fabioberger
approved these changes
Nov 14, 2019
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once the reference.mdx file changes are removed, LGTM
Made the changes suggested by @dekz and myself. |
dorothy-zbornak
pushed a commit
that referenced
this pull request
Feb 27, 2020
* update abi-gen with new method interfaces * wip: get all packages to build * wip: get all packages to build * Fix two contract wrapper calls * Export necessary types part of the contract wrapper public interfaces * Revive and fix wrapper_unit_tests * Remove duplicate type * Fix lib_exchange_rich_error_decoder tests * Fix remaining test failures in contracts-* packages * Prettier fixes * remove transactionHelper * lint and update changelogs * Fix prettier * Revert changes to reference docs * Add back changelog already published and add revert changelog entry * Add missing CHANGELOG entries * Add missing comma * Update mesh-rpc-client dep * Update Mesh RPC logic in @0x/orderbook to v6.0.1-beta * Align package versions
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
🚨This is a breaking change! 🚨
This PR changes wrapper method interfaces. Motivations:
a) Make wrapper docs size smaller. They are currently too large to upload
b) Make code smaller and more readable
c) Hopefully make methods easier to call and more abstractable
Old interface:
New interface:
🚨This is a breaking change! 🚨
This PR includes refactors to existing usages. We have a lot of open PRs right now in the leadup to V3 launch and code freeze. I've tried to reconcile changes as much as possible, but currently open PRs will have to be rebased and updated.
Useful regex-es for replacing (works in VisualStudio):
Do
Ctrl + Shift + H
and select theRegex
option. Use at your own risk! I recommend previewing changes before large batch replaces.Some gotchas:
web3Wrapper.awaitTransactionSuccessAsync
,web3Wrapper.sendTransactionAsync
someContract.someMethod(..., new BigNumber(1)).callAsync()
Impact on markdown size
BEFORE ALL:
contract-wrappers: 825kb
0x.js: 523kb
order-utils: 129kb
BEFORE (after cut more wrappers):
contract-wrappers: 694kb
0x.js: 505kb
order-utils: 89kb
AFTER (wrappers re-factor):
contract-wrappers: 344kb
0x.js: 274kb
order-utils: 89kb
contract-wrappers overall decline: 58%
0x.js: 47%
Testing instructions
CI should pass 🤞
Reviewers should give more attention to unusual function call patterns as there are more likely to be mistakes there. e.g. function calls abstracted away by test helpers.
Types of changes
Checklist:
[WIP]
if necessary.