Add dummy contract to test abi-gen-wrappers

[xianny]

We have been working on deprecating @0x/contract-wrappers by replacing it with code auto-generated by the abi-gen tool. To increase efficiency, we are trying to replace calls to pure Solidity functions with simulated EVM computations (#1872). As part of this push, it would be useful to have a dummy contract with different kinds of test functions so we can debug EVM evaluations in isolated unit tests.

Expected behaviour

A dummy contract implementing different kinds of pure Solidity functions, and corresponding unit tests. The intended workflow is:

1. Make desired changes to abi-gen or abi-gen-templates
2. Build the dummy contract package and generate wrappers for it
3. Run the unit tests to ensure the wrappers work as expected

cc @fabioberger

[feuGeneA]

This is great! abi-gen is completely lacking tests of its core functionality, and this is exactly what it needs.

I'm wondering if this enables us to stop checking in generated wrappers. We had said that a (the?) benefit of checking them in is that, when the handlebars templates change, you can see the resulting changes to the generated wrappers in the same PR. But that seems wrong to me, because it feels like we're using our production use case as a way to manually "test" code generation. Testing code generation should be done as part of the tests for the code generator, and this Issue is a great step in that direction.

Taking it a little further, I think we should check in the generated wrappers for this dummy contract fixture, as a "known-good" output. When the test runs, and produces freshly generated wrappers, they should be written somewhere else (as opposed to overwriting the known-good ones), and the yarn test command should include doing a diff of the known-good generation vs. the fresh generation.

If that seems like overkill, consider Python: While building and running the generated TypeScript is easy and therefore should be done, it's simply not practical to do that for Python, at least not within the context of running tests of abi-gen, because there isn't always a Python interpreter available in the Node.js environment. So, for Python, I think the right thing to do is create a diff test as described above. If I hear some agreement on that, I'll create an Issue to track it.

cc @fabioberger

[xianny]

Interesting idea @feuGeneA . I'm thinking about what should happen to that test output diff and here's my proposal:

• Check in the dummy generated wrappers
• Write a script for CI to regenerate dummy wrappers to a temp location using fresh builds of abi-gen* at the current commit
• If the temp wrappers are different from the checked in wrappers, fail CI

This means that anytime someone changes the abi-gen* packages, they are also compelled to regenerate and check in dummy generated wrappers. So this would expose any diffs to our regular git workflow and review process 🤞

Would this work with our Python setup?

[feuGeneA]

Sounds good! And yes, this would indeed work for Python.

Just one small nit to pick with your proposal as written: don't focus just on CI. Focus more broadly on yarn test, so that it's integrated with the existing local developer workflow too. (Maybe you're already thinking along those lines, but since you mentioned CI twice, and never yarn, I thought it was worth mentioning.)

[feuGeneA]

One more point of justification for this idea of using a diff test instead of checking in generated wrappers:

The way things are set up right now, someone who changes a handlebars template should also check in the generated wrappers that were affected by that change, but they don't have to. That is, there's no enforcement mechanism.

As a concrete example (and the impetus for this comment), I just pulled the latest changes from the development branch, and built abi-gen-wrappers, and now I have these changes in my working copy:

diff --git a/packages/abi-gen-wrappers/src/generated-wrappers/eth_balance_checker.ts b/packages/abi-gen-wrappers/src/generated-wrappers/eth_balance_checker.ts
index 94c5d1ad4..3c55f24af 100644
--- a/packages/abi-gen-wrappers/src/generated-wrappers/eth_balance_checker.ts
+++ b/packages/abi-gen-wrappers/src/generated-wrappers/eth_balance_checker.ts
@@ -64,6 +64,15 @@ export class EthBalanceCheckerContract extends BaseContract {
             // tslint:enable boolean-naming
             return result;
         },
+        getABIEncodedTransactionData(
+                addresses: string[],
+            ): string {
+            assert.isArray('addresses', addresses);
+            const self = this as any as EthBalanceCheckerContract;
+            const abiEncodedTransactionData = self._strictEncodeArguments('getEthBalances(address[])', [addresses
+        ]);
+            return abiEncodedTransactionData;
+        },
     };
     public static async deployFrom0xArtifactAsync(
         artifact: ContractArtifact | SimpleContractArtifact,

Whoever made the template changes that produced this wrapper change should have checked in this wrapper too, but apparently they missed it. Easy mistake, totally understand. But it may leave the next developer scratching their head wondering what they did to cause that change. Ending the practice of checking in generated wrappers would avoid this confusion.

Of course, with the diff test approach, there's still not necessarily an enforcement mechanism, because someone changing a template may neglect to cover those changes with tests. But at least that's something a reviewer can easily notice. And, there's no chance of the next developer finding confusing changes in freshly generated code.

[xianny]

@feuGeneA , agree that there should either a) be an enforcement mechanism to check in fresh generated wrappers or b) not check in generated wrappers at all.

For the example you mentioned, looks like it was just one wrapper that was missed in f2cbf4a. That commit was made before #1875 was merged, so abi-gen only checked for artifact updates to decide whether to regenerate. After #1875, abi-gen should check for artifact and template changes so we should see way fewer of these kinds of stray diffs.

I'm going to make the changes to testing that we discussed here, but leave abi-gen-wrappers as is pending further discussion.