-
Notifications
You must be signed in to change notification settings - Fork 465
Conversation
5744166
to
72021c7
Compare
72021c7
to
67f4803
Compare
fa10d8b
to
bafae3f
Compare
packages/abi-gen/templates/TypeScript/partials/method_abi_helper.handlebars
Show resolved
Hide resolved
@@ -48,6 +48,9 @@ export enum {{contractName}}Events { | |||
// tslint:disable:no-parameter-reassignment | |||
// tslint:disable-next-line:class-name | |||
export class {{contractName}}Contract extends BaseContract { | |||
/** | |||
* @ignore |
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.
tells typedoc to skip the next line when generating!
contracts/utils/test/ownable.ts
Outdated
@@ -49,7 +49,13 @@ describe('Ownable', () => { | |||
}); | |||
|
|||
it('should transfer ownership if the specified new owner is not the zero address', async () => { | |||
expect(ownable.transferOwnership.sendTransactionAsync(nonOwner, { from: owner })).to.be.fulfilled(''); | |||
expect( |
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.
not sure how this worked before 🤔
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.
My guess: Because the promise returned by sendTransactionAsync()
was indeed resolved, and ganache updated the "chain" super fast (it doesn't actually "mine" anything, right?), so fast that the subsequent .owner.callAsync()
was able to get the data it expected. I'd also guess that it would have failed intermittently, especially on a machine under high load.
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.
That makes sense. This is the first time I ran this test locally :)
af136a9
to
bafae3f
Compare
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.
If we're trying to make wrappers small again, and, given the bundle size bloat mentioned in the quote from @fabioberger , can we also delve into bundle size?
In particular, can we use this PR as a vehicle for discussing the recently introduced dependency on ethereumjs-vm
(for supporting local-machine execution of pure
Solidity functions)? @dekz was complaining to me about it last week. In informal conversation, he said ethereumjs-vm
added 2 MB to the bundle size. bundlephobia says it's 1.8 MB minified, and 569 KB minified and gzipped. In short, it's huge!
It would make a world of difference if we could make that functionality optional. But wouldn't we have to publish a separate package? Something like @0x/abi-gen-wrappers-evm
? What would we have to take into consideration in order to make this happen? And, what other approaches could we consider?
Also, I was thinking about another way to reduce bundle size. Since we're hardcoding ABI's into the generated wrappers, why can't we change the abi-gen-wrappers/package.json
to make @0x/contract-artifacts
be in dev
Dependencies
instead of a regular dependencies
? bundlephobia says @0x/contract-artifacts
is 380 KB minified, and 67 KB minified+gzipped. So, not a huge impact, but it seems like an easy win, no?
Also, a question about embedding deployedBytecode
in the wrappers: do we decide whether to embed it based on whether the contract actually has any pure functions? Or do we just embed it regardless? Some quick measurements show that by avoiding embedding for contracts that don't even have any pure functions, we can reduce total deployedBytecode
string length from 209 KB to 86 KB (gzipped size reduced from 21 KB to 7 KB). (I just duplicated contract-artifacts/lib
to lib2
, then rm
'd all artifacts that don't mention pure
, then did:
8Oct12:59:03 ~/dev/0x-monorepo-v3/packages/contract-artifacts[feat/3.0/python *$%]$ grep -A1 'deployedBytecode\": {' lib/artifacts/*.json | wc -m
214049
8Oct12:59:30 ~/dev/0x-monorepo-v3/packages/contract-artifacts[feat/3.0/python *$%]$ grep -A1 'deployedBytecode\": {' lib2/artifacts/*.json | wc -m
87847
8Oct12:59:31 ~/dev/0x-monorepo-v3/packages/contract-artifacts[feat/3.0/python *$%]$ grep -A1 'deployedBytecode\": {' lib2/artifacts/*.json | gzip | wc -m
7600
8Oct12:59:59 ~/dev/0x-monorepo-v3/packages/contract-artifacts[feat/3.0/python *$%]$ grep -A1 'deployedBytecode\": {' lib/artifacts/*.json | gzip | wc -m
21487
)
Perhaps at the very least (before embarking on any more actual work targeting reduced bundle size), could you update the Results section of your OP to show how bundle size has changed in light of the changes already present in this PR? I'm not an expert on our publishing process (do we minify, via webpack or something?), but I think a good proxy for bundle size is to just do du -h
on a package's lib
directory; and to get a gzipped size, you can do tar czO lib | wc -c
(read as: "use tar
to create (c
) a zipped (z
) file, printed to stdout (O
) and piped through the word count utility (wc
) to count bytes (-c
)).
(Done ranting about bundle size...)
Are those now-debug functions really just for debug? It seems like they're useful for other purposes too (though admittedly for more advanced/complex use cases), such as for example for preparing ABI-encoded calldata so that it can be signed locally before sending it off to a remote node for execution.
Also, as much as I hate to say this, the introduction of the --debug
flag exposes a weakness in our test-cli
setup. Specifically, our existing structure is only set up to test a single set of arguments to abi-gen
. Really, we should be testing all sorts of combinations: with and without --debug
, with and without --template
and --partials
, etc. Perhaps this is out of scope for this PR, but perhaps we should raise an Issue to discuss and specify how we could reasonably accomplish this.
packages/types/src/index.ts
Outdated
* shouldValidate: Flag indicating whether the library should make attempts to validate a transaction before | ||
* broadcasting it. For example, order has a valid signature, maker has sufficient funds, etc. Default=true. | ||
*/ | ||
export interface TxOpts { |
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.
I'm no expert on this, but I think this name incorrectly suggests that it relates directly to an Ethereum transaction, whereas this thing's usage is more indirect. Maybe SendTransactionOptions
, TxSendOptions
, or some variation thereof? 🤷♂️
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.
Ya good point. Going with SendTransactionOpts
(to match sendTransaction
method name)
contracts/utils/test/ownable.ts
Outdated
@@ -49,7 +49,13 @@ describe('Ownable', () => { | |||
}); | |||
|
|||
it('should transfer ownership if the specified new owner is not the zero address', async () => { | |||
expect(ownable.transferOwnership.sendTransactionAsync(nonOwner, { from: owner })).to.be.fulfilled(''); | |||
expect( |
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.
My guess: Because the promise returned by sendTransactionAsync()
was indeed resolved, and ganache updated the "chain" super fast (it doesn't actually "mine" anything, right?), so fast that the subsequent .owner.callAsync()
was able to get the data it expected. I'd also guess that it would have failed intermittently, especially on a machine under high load.
pollingIntervalMs, | ||
timeoutMs, | ||
opts.pollingIntervalMs, | ||
opts.timeoutMs, |
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.
Should we also update awaitTransactionSuccessAsync()
to also take a TxOpts
object?
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.
I intentionally didn't change web3wrapper
because the arguments for awaitTransactionSuccessAsync(txHash, pollingIntervalMs, timeoutMs)
don't have overlap with sendTransactionAsync(txData)
. They are pretty different steps.
awaitTransactionSuccessAsync
in the contract wrappers has been updated to take TxOpts
though.
In the contract-wrapper class, awaitTransactionSuccessAsync
is a convenience that calls web3Wrapper.sendTransactionAsync(txData)
, retrieves the txhash, then calls web3Wrapper.awaitTransactionSuccessAsync(txHash)
. Thus sendTransactionAsync()
is a subset of awaitTransactionSuccessAsync()
. I created the TxOpts
object to 1. further abstract the differences between the two away so a developer can call both with the same arguments, and 2. allow us more flexibility to add or remove optional args later, but those dont apply for web3wrapper
.
async sendTransactionAsync( | ||
owner: string, | ||
txData?: Partial<TxData> | undefined, | ||
opts: TxOpts = { shouldValidate: true }, |
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.
Just pointing out for other reviewers here: we're changing the default behavior of this (generated) function such that now it will do an eth_call
first, and then actually do an eth_sendTranscation
, whereas before we had a separate method to do that combo (validateAndSendTransactionAsync()
). Is that what we want? It makes sense to me, but I wanted to be sure to raise the visibility of this change.
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.
This was also the default behaviour a few months back before we started the whole multi-lang/abi-gen project. Neither @feuGeneA nor myself understood the importance of performing an eth_call
first so I treated it as an optional step! But have since learned that it's a good idea to always validate before sending a tx.
@@ -73,12 +77,11 @@ const args = yargs | |||
default: 'TypeScript', | |||
}) | |||
.example( | |||
"$0 --abis 'src/artifacts/**/*.json' --out 'src/contracts/generated/' --partials 'src/templates/partials/**/*.handlebars' --template 'src/templates/contract.handlebars'", | |||
"$0 --abis 'src/artifacts/**/*.json' --out 'src/contracts/generated/' --debug --partials 'src/templates/partials/**/*.handlebars' --template 'src/templates/contract.handlebars'", |
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.
Definitely a nit, but I'd prefer to make the example as simple as possible, so I would skip the --debug
here, and I would actually advocate for removing --partials
and --handlebars
as well, though admittedly that doesn't necessarily feel in context for this PR.
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.
Hmmmm shouldn't we make the example use all the options so that it provides more complete info? Folks can ignore whatever doesn't apply to them.
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.
Definitely a matter of opinion. I think of it as a one-liner "quick start guide", and the easiest way to "get started" is by accepting the defaults. People can always find usages "in the wild" (eg our other packages) if they want to see more complex examples. I'm open to having my mind changed though.
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.
I think I agree with Xianny that a more complete example is potentially more helpful. It's easier to remove then to know how to add.
packages/abi-gen/src/index.ts
Outdated
@@ -44,6 +44,10 @@ const args = yargs | |||
normalize: true, | |||
demandOption: true, | |||
}) | |||
.option('debug', { | |||
describe: 'Enable debug functions', | |||
type: 'count', |
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.
why not
type: 'count', | |
type: 'boolean', |
?
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.
For some reason I thought that would imply --debug=true
or --debug=false
, but you are totally right, it works the same. Thank you!
packages/abi-gen/src/index.ts
Outdated
@@ -425,6 +428,7 @@ for (const abiFileName of abiFileNames) { | |||
ABIString: JSON.stringify(ABI), | |||
methods: methodsData, | |||
events: eventsData, | |||
debug: args.debug > 0, |
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.
If args.debug
should be type: "boolean"
, then this should just be
debug: args.debug > 0, | |
debug: args.debug, |
...right?
import { Web3Wrapper } from '@0x/web3-wrapper'; | ||
import { assert } from '@0x/assert'; | ||
import * as ethers from 'ethers'; | ||
// tslint:enable:no-unused-variable | ||
|
||
export type AbiGenDummyEventArgs = AbiGenDummyWithdrawalEventArgs | AbiGenDummySimpleEventEventArgs; | ||
export type AbiGenDummyEventArgs = AbiGenDummySimpleEventEventArgs | AbiGenDummyWithdrawalEventArgs; |
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.
Is this ordering nondeterministic?
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.
It's deterministic but the latest version of sol-compiler uses soljson-v0.5.12
, whereas the existing artifacts were compiled with soljson-v0.5.10
. So I assume this has something to do with the ordering.
I actually found an existing method in the dummy contract that let me do what I needed, so I removed the contract changes and reverted the artifact. We should open a separate PR just to update the artifacts though so that future diffs will be clean. I'll open an issue and do it after this one is merged. #2251
return abiDecodedReturnData; | ||
}, | ||
}; | ||
public methodReturningArrayOfStructs = { | ||
public simplePureFunction = { |
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.
Is the order of methods in the generated wrapper non-deterministic? That would be really unfortunate... It seems like there's a ton of "noise" in this diff due simply to a re-ordering of method declarations. If that's the case, we should consider sorting them before generating them. Out of scope for this PR of course, but maybe we should make an Issue about it.
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.
see #2243 (comment)
@@ -184,7 +184,7 @@ export class BaseContract { | |||
} | |||
return rawEncoded; | |||
} | |||
public async evmExecAsync(input: Buffer): Promise<string> { | |||
protected async _evmExecAsync(input: Buffer): Promise<string> { |
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.
Is the reason for this just to avoid having documentation for it generated into the markdown files? Or is there some other reason as well?
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.
Partly to avoid having docs generated, partly because it's not used anywhere else, so follows the principle of minimum exposed surfaces.
It doesn't make a whole lotta sense to call it in an external context. It only works for functions defined in the Solidity and available via bytecode; and those should all be accessed by calling functionName.callAsync
or similar. I have a feeling we should also avoid bypassing those to directly pass in callData even for unit tests because that would be an easy way to create holes in our testing.
5e08cbc
to
e2568b8
Compare
* revert artifacts to get rid of ordering diffs * trim deployedBytecode so it's undefined unless a contract has pure functions * rename TxOpts -> SendTransactionOpts * create sendTransactionOptsSchema in json-schemas * fix dependencies in @0x/abi-gen-wrappers
e2568b8
to
0383ebd
Compare
Thanks for looking into the bundle size @feuGeneA ! I agree that the bundle is pretty large right now and a lot of it is due to
Re: ethereumjs-vm dependency, yeah it's huge :/ but the previous consensus has been that it's worth it. Maybe we need to revisit this, but that seems out of scope for this PR. From the POV of an external developer I'm also not 100% convinced that I would prefer to use |
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.
Drop JSON schema validations. Otherwise everything looks good to me!
@@ -420,15 +420,17 @@ for (const abiFileName of abiFileNames) { | |||
return eventData; | |||
}); | |||
|
|||
const shouldIncludeBytecode = methodsData.find(methodData => methodData.stateMutability === 'pure') !== undefined; |
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.
❤️
contracts/utils/test/ownable.ts
Outdated
ownable.transferOwnership.awaitTransactionSuccessAsync( | ||
nonOwner, | ||
{ from: owner }, | ||
{ timeoutMs: constants.AWAIT_TRANSACTION_MINED_MS }, |
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.
There is a default value for this right?
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.
Now there is :)
packages/abi-gen/src/index.ts
Outdated
@@ -44,6 +44,10 @@ const args = yargs | |||
normalize: true, | |||
demandOption: true, | |||
}) | |||
.option('debug', { | |||
describe: 'Enable debug functions', |
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.
describe: 'Enable debug functions', | |
describe: 'Includes debug functions in the wrappers such as `getABIDecodedTransactionData`', |
@@ -73,12 +77,11 @@ const args = yargs | |||
default: 'TypeScript', | |||
}) | |||
.example( | |||
"$0 --abis 'src/artifacts/**/*.json' --out 'src/contracts/generated/' --partials 'src/templates/partials/**/*.handlebars' --template 'src/templates/contract.handlebars'", | |||
"$0 --abis 'src/artifacts/**/*.json' --out 'src/contracts/generated/' --debug --partials 'src/templates/partials/**/*.handlebars' --template 'src/templates/contract.handlebars'", |
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.
I think I agree with Xianny that a more complete example is potentially more helpful. It's easier to remove then to know how to add.
packages/abi-gen/templates/TypeScript/partials/method_abi_helper.handlebars
Show resolved
Hide resolved
@@ -8,6 +8,7 @@ | |||
async sendTransactionAsync( | |||
{{> typed_params inputs=inputs}} | |||
txData?: Partial<TxData> | undefined, | |||
opts: SendTransactionOpts = { shouldValidate: true }, |
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.
It's a bit strange that if you pass in pollingIntervalMs
or timeoutMs
here, it won't do anything and doesn't really make sense. Probably cleaner to have a separate opts type for this method and awaitTransactionSuccessAsync
. The one for awaitTransactionSuccessAsync
can be a super-set of the type for sendTransactionAsync
.
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.
That makes complete sense. I kind of forgot about TypeScript type system ^^"
|
||
export interface IndexedFilterValues { | ||
[index: string]: ContractEventArg; | ||
} |
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.
These were all duplicated in @0x/types
4d52d83
to
630e4c6
Compare
* Create `AwaitTransactionSuccessOpts` type with default timeoutMs value * Remove duplicate types `IndexedFilterValues`, `DecodedLogEvent`, `EventCallback` from `@0x/base-contract` * Better comments
630e4c6
to
805c9bc
Compare
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.
Please address my one last comment before merging.
@@ -323,7 +329,7 @@ export class AbiGenDummyContract extends BaseContract { | |||
awaitTransactionSuccessAsync( | |||
wad: BigNumber, | |||
txData?: Partial<TxData>, | |||
opts: SendTransactionOpts = { shouldValidate: true }, | |||
opts: AwaitTransactionSuccessOpts = { shouldValidate: true, timeoutMs: 0 }, |
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.
Defaulting timeoutMs
to 0 doesn't make any sense. From the doc comment, this is: How long (in ms) to poll for transaction mined until aborting.
. Aborting after 0 MS makes it impossible for the poll request to complete. Am I missing something?
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.
Ahh good catch. This should be pollingIntervalMs
and was only set to 0 in contracts unit tests. @0x/web3wrapper
has a default pollingIntervalMs = 1000
already so I will leave the wrapper without a default value and keep the existing behaviour.
f3cf65e
to
7e30376
Compare
…ided in web3wrapper
7e30376
to
3750545
Compare
* introduce --debug option to abi-gen and remove debug functions from @0x/abi-gen-wrappers * make evmExecAsync protected; ignore deployedBytecode in doc comment * trim deployedBytecode so it's undefined unless a contract has pure functions * remove validateAndSendTransactionAsync * Create `AwaitTransactionSuccessOpts` and `SendTransactionOpts` types * Remove duplicate types `IndexedFilterValues`, `DecodedLogEvent`, `EventCallback` from `@0x/base-contract`
Description
I'm tagging a long list of reviewers to include everyone who might need to know about these changes. Protocol team, please note interface changes to
sendTransactionAsync
andawaitTransactionSuccessAsync
.Changes:
evmExecAsync
protecteddeployedBytecode
deployedBytecode
so it's undefined unless a contract as pure functionsgetABIEncodedTransactionData
for non-constant functions, or if in debug modegetABIDecodedTransactionData
andgetABIDecodedReturnData
in debug modevalidateAndSendTransactionAsync
. Allow to provide ashouldValidate
optional argument insteadSendTransactionOpts
type for non-ethereum transaction options:Debug mode is enabled for all
contracts/*
packages as they are not published.Results
0x.js MD file: 849KB --> 509KB
Contract-wrappers MD file: 1.4MB --> 811KB
abi-gen-wrappers
lib
size: 5.0MB -> 3.7 MB (3.8 without trimming bytecode)Testing instructions
Types of changes
Checklist:
[WIP]
if necessary.