Decode log arguments in `awaitTransactionSuccessAsync` #1995

hysz · 2019-07-25T16:49:22Z

Description

The awaitTransactionSuccessAsync function in generated contract wrappers will decode logs, but requires ABI definitions to map topics to arguments. To do this we need (i) the abi for the wrapped contract and (ii) the abi for any contracts it calls.

This PR introduces a new parameter, abiDependencies, to the constructor of generated contract wrappers. This is passed in when we deploy the contract (which instantiates the wrapper):

// Old
const exchange = await ExchangeContract.deployFrom0xArtifactAsync(
    artifacts.Exchange,
    provider,
    txDefaults,
    zrxTokenAddress
);

// New
const dependencyArtifacts = {
    ...erc20Artifacts,
    ...erc721Artifacts,
    ...erc1155Artifacts,
};
const exchange = await ExchangeContract.deployFrom0xArtifactAsync(
    artifacts.Exchange,
    provider,
    txDefaults,
    dependencyArtifacts,
    zrxTokenAddress
);

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.

buildsize · 2019-07-26T16:33:56Z

File name	Previous Size	New Size	Change
init.py	74.45 KB	74.45 KB	0 bytes (0%)
abi_gen_dummy.ts	64.99 KB	[deleted]
lib_dummy.ts	4.08 KB	[deleted]
test_lib_dummy.ts	9.06 KB	[deleted]
environment.pickle	1.56 MB	1.56 MB	0 bytes (0%)
index.doctree	189.46 KB	189.46 KB	0 bytes (0%)
.buildinfo	230 bytes	230 bytes	0 bytes (0%)
genindex.html	5.6 KB	5.6 KB	0 bytes (0%)
index.html	2.52 KB	2.52 KB	0 bytes (0%)
objects.inv	375 bytes	375 bytes	0 bytes (0%)
py-modindex.html	3.07 KB	3.07 KB	0 bytes (0%)
search.html	2.81 KB	2.81 KB	0 bytes (0%)
searchindex.js	5.81 KB	5.81 KB	0 bytes (0%)
index.rst.txt	415 bytes	415 bytes	0 bytes (0%)
alabaster.css	10.92 KB	10.92 KB	0 bytes (0%)
basic.css	11.83 KB	11.83 KB	0 bytes (0%)
custom.css	42 bytes	42 bytes	0 bytes (0%)
doctools.js	9.05 KB	9.05 KB	0 bytes (0%)
documentation_options.js	303 bytes	303 bytes	0 bytes (0%)
file.png	286 bytes	286 bytes	0 bytes (0%)
jquery-[version].js	261.76 KB	261.76 KB	0 bytes (0%)
jquery.js	84.63 KB	84.63 KB	0 bytes (0%)
language_data.js	10.59 KB	10.59 KB	0 bytes (0%)
minus.png	90 bytes	90 bytes	0 bytes (0%)
plus.png	90 bytes	90 bytes	0 bytes (0%)
pygments.css	4.69 KB	4.69 KB	0 bytes (0%)
searchtools.js	15.61 KB	15.61 KB	0 bytes (0%)
underscore-[version].js	34.34 KB	34.34 KB	0 bytes (0%)
underscore.js	11.86 KB	11.86 KB	0 bytes (0%)
contract_addresses.html	16.87 KB	16.87 KB	0 bytes (0%)
contract_artifacts.html	7.94 KB	7.94 KB	0 bytes (0%)
_bootstrap.html	142.73 KB	142.73 KB	0 bytes (0%)
json_schemas.html	12.43 KB	12.43 KB	0 bytes (0%)
order_utils.html	44.87 KB	44.87 KB	0 bytes (0%)
erc20_token.html	43.57 KB	43.57 KB	0 bytes (0%)
exchange.html	218.79 KB	218.79 KB	0 bytes (0%)
tx_params.html	8.83 KB	8.83 KB	0 bytes (0%)
local_message_signer.html	15.07 KB	15.07 KB	0 bytes (0%)
asset_data_utils.html	22.65 KB	22.65 KB	0 bytes (0%)
default_api.html	118.49 KB	118.49 KB	0 bytes (0%)

coveralls · 2019-07-26T18:00:51Z

Coverage increased (+0.005%) to 82.074% when pulling 6f64115 on feature/ContractWrappers/decodeLogArgs into aa29526 on development.

jalextowle · 2019-07-26T18:08:01Z

contracts/exchange/test/utils/exchange_wrapper.ts

@@ -251,15 +222,14 @@ export class ExchangeWrapper {
        from: string,
    ): Promise<TransactionReceiptWithDecodedLogs> {
        const params = orderUtils.createMatchOrders(signedOrderLeft, signedOrderRight);
-        const txHash = await this._exchange.matchOrders.sendTransactionAsync(
+        const txReceipt = await this._exchange.matchOrders.awaitTransactionSuccessAsync(


This is very nice 🎉

jalextowle · 2019-07-26T18:09:06Z

contracts/utils/contracts/test/TestLogDecodingDownstream.sol

+    /// For testing, we want to emit an event that is
+    /// not known by the calling contract.
+    event TestEvent2(
+        uint256 lorem,


I love the lorem ipsum 👍

jalextowle · 2019-07-26T18:12:41Z

contracts/utils/test/log_decoding.ts

+            // tslint:disable no-unnecessary-type-assertion
+            expect((txReceipt.logs[0] as LogWithDecodedArgs<DecodedLogArgs>).args).to.be.deep.equal(expectedEvent);
+        });
+        it('should not event args when no dependencies are passed into wrapper', async () => {


"should not decode"?

jalextowle

Looks good to me. I'm looking forward to using this stuff!

dorothy-zbornak

Looks fantastic!
just a small request then g2g.

dorothy-zbornak · 2019-07-26T18:20:06Z

contracts/utils/test/log_decoding.ts

+        await blockchainLifecycle.revertAsync();
+    });
+
+    describe('Decoding Log Arguments', () => {


It isn't immediately clear from the tests what the behavior is when we do an emitEvent() and emitEventDownstream() at once, without downstream artifacts. I assume TestEvent will turn into a DecodedLogEntry and TestEvent2 will remain a LogEntry, but it might be useful to demonstrate it.

Heh I actually had a test for that and removed it. I'll put it back in. 🙏

fabioberger · 2019-07-26T21:14:55Z

packages/abi-gen-wrappers/src/generated-wrappers/erc20_token.ts

@@ -583,6 +584,7 @@ export class ERC20TokenContract extends BaseContract {
        artifact: ContractArtifact | SimpleContractArtifact,
        supportedProvider: SupportedProvider,
        txDefaults: Partial<TxData>,
+        artifactDependencies: { [contractName: string]: ContractArtifact | SimpleContractArtifact },


Shouldn't this be named contractDependenciesArtifacts? You are passing in the artifacts of the dependencies of the contract no? artifactDependencies made me think these are dependencies of the artifacts -- which I admit is non-sensical.

xianny

We discussed on Slack if there were alternatives to passing the ABIs in the constructor, and agreed this is probably the best way to do this at this time.

One option discussed was to create a standalone module that exports a static mapping of <contract name>: [ ... contract dependencies]. When instantiating, check this mapping for any default dependencies. We decided that was overkill in this case.

I'm also not super happy with the name abiDependencies. I think abi is already overused and confusing. What about contractDependencies? The type annotation of contractName -> ContractAbi makes it clear that we are expecting an ABI.

I also have a small nit about introducing new lodash use in abi-gen. Apart from these two, looks good to me. :)

packages/abi-gen/test-cli/expected-output/typescript/abi_gen_dummy.ts

packages/abi-gen/test-cli/expected-output/typescript/lib_dummy.ts

hysz · 2019-07-27T10:59:11Z

Thanks @fabioberger @xianny, I appreciate the feedback!

To provide more context, the artifacts/abis are dependencies of the wrapper but not necessarily of the underlying contract. Moreover, they’re known by the end user of the wrapper but may not be known to the author of the contract.

Here are two examples of when these dependencies are needed by the wrapper:

At present these artifacts are used solely to decode logs, although I used more general naming to try and future proof the parameter. That said, it sounds like this naming is too ambiguous and may not be prescriptive enough. What if we go with something like,logDecodeDependencies?

fabioberger · 2019-07-27T11:26:58Z

Thanks for the explanation @hysz. I prefer logDecodeDependencies personally. Let's also make sure to add a very good explanation in the doc comment for that method, to further educate the end dev on what it's for.

…ecognized.

… are included/excluded

…m0xArtifactAsync` and `deployAsync`

…downstream events

… + removed lodash depency in contract wrappers

… and to pass linter

xianny

thanks for the detailed explanation @hysz ! looks good to me :)

hysz requested review from abandeali1, feuGeneA and xianny as code owners July 25, 2019 16:49

hysz requested a review from fabioberger as a code owner July 26, 2019 15:52

hysz force-pushed the feature/ContractWrappers/decodeLogArgs branch 2 times, most recently from 8b67e2d to 23188d7 Compare July 26, 2019 16:27

hysz changed the title ~~[WIP] Decode log arguments in awaitTransactionSuccessAsync~~ Decode log arguments in awaitTransactionSuccessAsync Jul 26, 2019

hysz assigned dorothy-zbornak and xianny Jul 26, 2019

jalextowle reviewed Jul 26, 2019

View reviewed changes

jalextowle approved these changes Jul 26, 2019

View reviewed changes

dorothy-zbornak approved these changes Jul 26, 2019

View reviewed changes

fabioberger reviewed Jul 26, 2019

View reviewed changes

xianny suggested changes Jul 26, 2019

View reviewed changes

packages/abi-gen/test-cli/expected-output/typescript/abi_gen_dummy.ts Outdated Show resolved Hide resolved

packages/abi-gen/test-cli/expected-output/typescript/lib_dummy.ts Outdated Show resolved Hide resolved

fabioberger approved these changes Jul 27, 2019

View reviewed changes

hysz added 9 commits July 29, 2019 12:04

Decode log arguments in awaitTransactionSuccessAsync, when ABI is r…

df746c5

…ecognized.

Updated all instances of deployFrom0xArtifactAsync

d8cf9d5

Updated changelogs for deployFrom0xArtifactAsync interface change

88a7d9c

Added tests for for decoding log arguments when artifact dependencies…

365c056

… are included/excluded

Updated generated contract wrappers and ran linter

2478310

Minor readability improvements

2968dfb

updated test output from abi-gen to account for changes to `deployFro…

ea8fc1d

…m0xArtifactAsync` and `deployAsync`

Added another test to show what happens when decoding both local and …

58e0833

…downstream events

fixed typo in a test

8961b47

hysz added 2 commits July 29, 2019 12:19

Changed artifactDependencies/abiDependencies to logDecodeDependencies…

c2f34ba

… + removed lodash depency in contract wrappers

updated contract wrappers

048e48b

hysz force-pushed the feature/ContractWrappers/decodeLogArgs branch from 85934f3 to 048e48b Compare July 29, 2019 10:25

hysz added 4 commits July 29, 2019 12:28

Updated expected typescript output for abi-gen cli tests

a8939d3

ran prettier on abi-gen-wrappers

26ee4d6

Fixed linter errors

0aadb78

Switched assignment of logDecodeDependenciesAbiOnly for readability…

6f64115

… and to pass linter

xianny approved these changes Jul 29, 2019

View reviewed changes

hysz merged commit 7cba95b into development Jul 29, 2019

snyk-bot mentioned this pull request Nov 12, 2021

[Snyk] Security upgrade @0x/contracts-test-utils from 2.0.1 to 3.1.12 Matthelonianxl/0x-monorepo#16

Open

This was referenced Feb 4, 2022

[Snyk] Security upgrade @0x/contracts-test-utils from 2.0.1 to 3.1.12 Matthelonianxl/0x-monorepo#24

Open

[Snyk] Fix for 13 vulnerabilities Matthelonianxl/0x-monorepo#26

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decode log arguments in `awaitTransactionSuccessAsync` #1995

Decode log arguments in `awaitTransactionSuccessAsync` #1995

hysz commented Jul 25, 2019 •

edited

Loading

buildsize bot commented Jul 26, 2019 •

edited

Loading

coveralls commented Jul 26, 2019 •

edited

Loading

jalextowle Jul 26, 2019

jalextowle Jul 26, 2019

jalextowle Jul 26, 2019

jalextowle left a comment

dorothy-zbornak left a comment

dorothy-zbornak Jul 26, 2019

hysz Jul 26, 2019

fabioberger Jul 26, 2019 •

edited

Loading

xianny left a comment

hysz commented Jul 27, 2019

fabioberger commented Jul 27, 2019

xianny left a comment

Decode log arguments in awaitTransactionSuccessAsync #1995

Decode log arguments in awaitTransactionSuccessAsync #1995

Conversation

hysz commented Jul 25, 2019 • edited Loading

Description

Testing instructions

Types of changes

Checklist:

buildsize bot commented Jul 26, 2019 • edited Loading

coveralls commented Jul 26, 2019 • edited Loading

jalextowle Jul 26, 2019

Choose a reason for hiding this comment

jalextowle Jul 26, 2019

Choose a reason for hiding this comment

jalextowle Jul 26, 2019

Choose a reason for hiding this comment

jalextowle left a comment

Choose a reason for hiding this comment

dorothy-zbornak left a comment

Choose a reason for hiding this comment

dorothy-zbornak Jul 26, 2019

Choose a reason for hiding this comment

hysz Jul 26, 2019

Choose a reason for hiding this comment

fabioberger Jul 26, 2019 • edited Loading

Choose a reason for hiding this comment

xianny left a comment

Choose a reason for hiding this comment

hysz commented Jul 27, 2019

fabioberger commented Jul 27, 2019

xianny left a comment

Choose a reason for hiding this comment

Decode log arguments in `awaitTransactionSuccessAsync` #1995

Decode log arguments in `awaitTransactionSuccessAsync` #1995

hysz commented Jul 25, 2019 •

edited

Loading

buildsize bot commented Jul 26, 2019 •

edited

Loading

coveralls commented Jul 26, 2019 •

edited

Loading

fabioberger Jul 26, 2019 •

edited

Loading