This repository has been archived by the owner on Jul 9, 2021. It is now read-only.
switch @0x/contract-wrappers to generated wrappers #2037
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
multilang/deprecate-contract-wrappers
branch
3 times, most recently
from
a0a0763 to
49fa73b
Compare
|
xianny
force-pushed
the
multilang/deprecate-contract-wrappers
branch
from
49fa73b to
79b4ae3
Compare
xianny
force-pushed
the
multilang/deprecate-contract-wrappers
branch
2 times, most recently
from
c66935c to
01ff674
Compare
xianny
commented
Aug 7, 2019
| @@ -48,7 +48,6 @@ | |||
| "homepage": "https://github.com/0xProject/0x-monorepo/contracts/dev-utils/README.md", | |||
| "devDependencies": { | |||
| "@0x/abi-gen": "^3.1.2", | |||
| "@0x/contract-wrappers": "^10.1.0", | |||
There was a problem hiding this comment.
this package wasn't using contract-wrappers
xianny
commented
Aug 7, 2019
| @@ -47,7 +47,6 @@ | |||
| "homepage": "https://github.com/0xProject/0x-monorepo/contracts/extensions/README.md", | |||
| "devDependencies": { | |||
| "@0x/abi-gen": "^3.1.2", | |||
| "@0x/contract-wrappers": "^10.1.0", | |||
There was a problem hiding this comment.
this package wasn't using contract-wrappers
xianny
commented
Aug 7, 2019
| @@ -48,7 +48,7 @@ | |||
| "homepage": "https://github.com/0xProject/0x-monorepo/contracts/extensions/README.md", | |||
| "devDependencies": { | |||
| "@0x/abi-gen": "^3.1.2", | |||
| "@0x/contract-wrappers": "^10.1.0", | |||
| "@0x/contract-wrappers": "10.1.0", | |||
There was a problem hiding this comment.
pinning the version to do a mass refactor later. Some packages will be complicated to refactor, and before publishing, we can't specify 11.0.0 in some packages and 10.1.0 in others. So for now all packages will still use 10.1.0.
xianny
commented
Aug 7, 2019
| @@ -50,7 +50,7 @@ | |||
| "config": { | |||
| "contractsPackages": "@0x/contracts-asset-proxy @0x/contracts-erc20 @0x/contracts-erc721 @0x/contracts-erc1155 @0x/contracts-exchange @0x/contracts-exchange-forwarder @0x/contracts-exchange-libs @0x/contracts-extensions @0x/contracts-multisig @0x/contracts-test-utils @0x/contracts-utils @0x/contracts-coordinator @0x/contracts-dev-utils", | |||
| "mnemonic": "concert load couple harbor equip island argue ramp clarify fence smart topic", | |||
| "packagesWithDocPages": "0x.js connect json-schemas subproviders web3-wrapper contract-wrappers order-utils order-watcher sol-compiler sol-coverage sol-profiler sol-trace ethereum-types asset-buyer migrations", | |||
| "packagesWithDocPages": "0x.js connect json-schemas subproviders web3-wrapper order-utils order-watcher sol-compiler sol-coverage sol-profiler sol-trace ethereum-types asset-buyer migrations", | |||
xianny
commented
Aug 7, 2019
| ERC721ProxyContract, | ||
| ExchangeContract, | ||
| ForwarderContract, | ||
| OrderValidatorContract, |
There was a problem hiding this comment.
test:docs_generate complains if we don't include this
xianny
commented
Aug 7, 2019
| if (Object.keys(logDecodeDependencies) !== undefined) { | ||
| for (const key of Object.keys(logDecodeDependencies)) { | ||
| logDecodeDependenciesAbiOnly[key] = logDecodeDependencies[key].compilerOutput.abi; | ||
| } |
There was a problem hiding this comment.
This was throwing runtime errors.
xianny
commented
Aug 7, 2019
| @@ -54,22 +52,6 @@ To build this package and all other monorepo packages that it depends on, run th | |||
| PKG=@0x/contract-wrappers yarn build | |||
| ``` | |||
|
|
|||
| Or continuously rebuild on change: | |||
There was a problem hiding this comment.
doesn't make sense to continuously rebuild since it's generated
xianny
commented
Aug 7, 2019
| @@ -87,3 +69,9 @@ yarn lint | |||
| ```bash | |||
| yarn test | |||
| ``` | |||
|
|
|||
| ### Documentation | |||
There was a problem hiding this comment.
Copied from the existing abi-gen-wrappers README without changes.
xianny
commented
Aug 7, 2019
| import { AsyncMethod, ContractWrappersError, SyncMethod } from '../types'; | ||
|
|
||
| import { constants } from './constants'; | ||
| export enum ContractWrappersError { |
There was a problem hiding this comment.
Can we get rid of this now or should we wait for V3 better revert reasons? This file mainly converts Ethereum error msgs (e.g. 'out of gas') into these standardised error codes.
xianny
force-pushed
the
multilang/deprecate-contract-wrappers
branch
from
01ff674 to
ad9e2c9
Compare
xianny
force-pushed
the
multilang/deprecate-contract-wrappers
branch
from
ad9e2c9 to
e57d29a
Compare
xianny
requested review from
abandeali1,
BMillman19,
fragosti and
hysz
as code owners
xianny
removed request for
steveklebanoff,
BMillman19,
fragosti,
hysz and
abandeali1
fabioberger
suggested changes
Aug 7, 2019
| @@ -161,9 +115,8 @@ export class ContractWrappers { | |||
| */ | |||
| public unsubscribeAll(): void { | |||
| this.exchange.unsubscribeAll(); | |||
| this.erc20Token.unsubscribeAll(); | |||
| this.erc721Token.unsubscribeAll(); | |||
| this.etherToken.unsubscribeAll(); | |||
There was a problem hiding this comment.
Is the WETH contract not included?
There was a problem hiding this comment.
The etherTokenWrapper looked like a wrapper around potentially many WETH9 tokens. Users had to pass in the token address for each method call.
_etherTokenContractsByAddress: {
[address: string]: WETH9Contract;
} = {};
...
public async depositAsync(
etherTokenAddress: string,
amountInWei: BigNumber,
depositor: string,
txOpts: TransactionOpts = {},
): Promise<string> {
...
Should we change it to work with just the WETH9 contract deployed at addresses in @0x/contract-addresses?
There was a problem hiding this comment.
erc20tokenWrapper and erc721tokenWrapper were similar and I just left those out.
There was a problem hiding this comment.
Yeah, let's do with the WETH contract address in contract-addresses. If they want something else, they can instantiate it themselves.
| export { OrderValidatorWrapper } from './contract_wrappers/order_validator_wrapper'; | ||
| export { DutchAuctionWrapper } from './contract_wrappers/dutch_auction_wrapper'; | ||
|
|
||
| export { CoordinatorWrapper } from './coordinator_wrapper'; | ||
| export { TransactionEncoder } from './utils/transaction_encoder'; |
There was a problem hiding this comment.
Where is TransactionEncoder still used? I thought this would be replaced with calls to getABIEncodedTransactionData.
There was a problem hiding this comment.
You're right, it was still being used in coordinator. removing.
| import { AsyncMethod, ContractWrappersError, SyncMethod } from '../types'; | ||
|
|
||
| import { constants } from './constants'; | ||
| export enum ContractWrappersError { |
feuGeneA
approved these changes
Aug 7, 2019
| @@ -4,4 +4,5 @@ | |||
| *.svg linguist-generated=true | |||
| packages/contract-artifacts/artifacts/*json linguist-generated=true | |||
| packages/abi-gen-wrappers/src/generated-wrappers/*.ts linguist-generated=true | |||
| packages/contract-wrappers/src/generated-wrappers/*.ts linguist-generated=true | |||
| * erc20Proxy smart contract. | ||
| */ | ||
| public erc20Proxy: ERC20ProxyWrapper; | ||
| public erc20Proxy: ERC20ProxyContract; |
There was a problem hiding this comment.
Oh how I so love to see the word "wrapper" go away!! 😄
- remove TransactionEncoder - move TokenUtils to @0x/dev-utils
xianny
force-pushed
the
multilang/deprecate-contract-wrappers
branch
from
ee9b054 to
9ea1936
Compare
fabioberger
approved these changes
Aug 7, 2019
* fix getAbiEncodedTransactionData convenience method * export WETH9Contract from 0x.js * Update CHANGELOG to point to new issue
xianny
force-pushed
the
multilang/deprecate-contract-wrappers
branch
from
dd6c0c9 to
92f0dd7
Compare
This was referenced Aug 19, 2019
dorothy-zbornak
pushed a commit
that referenced
this pull request
Feb 27, 2020
* switch @0x/contract-wrappers to generated wrappers - remove TransactionEncoder - move TokenUtils to @0x/dev-utils - detailed changes in #2040
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
Switch to using generated wrappers in
@0x/contract-wrappers.This PR reproduces the factory class
ContractWrappersto help interface with the individual contracts.This PR does not refactor the current uses of
contract-wrappersin the monorepo. Most cases are easy to refactor but there are a couple cases where we will need to implement a workaround. Before publishing the new version ofcontract-wrappers, we can't specify new vs old version for existing monorepo dependencies, so it will be easier to update them all at the same time and after we have published the new version. There are 5 packages that currently usecontract-wrappers:This PR does not get rid of
abi-gen-wrappers. We will remove it after monorepo use cases have been refactored out.This follow-up work is tracked here: #1989
Testing instructions
I've kept the existing manually written exchange_wrapper_test, which works.
Types of changes
Checklist:
[WIP]if necessary.