assume type is address if type is unknown #291

dbeal-eth · 2020-10-28T23:35:21Z

When given a abi definition with a type defined as such:

{
      "inputs": [
        {
          "internalType": "contract IConfigurableRightsPool",
          "name": "self",
          "type": "IConfigurableRightsPool"
        },
        {
          "internalType": "contract IBPool",
          "name": "bPool",
          "type": "IBPool"
        },
        {
          "internalType": "uint256",
          "name": "poolAmountIn",
          "type": "uint256"
        },
        {
          "internalType": "uint256[]",
          "name": "minAmountsOut",
          "type": "uint256[]"
        }
      ],
      "name": "exitPool",
      "outputs": [
        {
          "internalType": "uint256",
          "name": "exitFee",
          "type": "uint256"
        },
        {
          "internalType": "uint256",
          "name": "pAiAfterExitFee",
          "type": "uint256"
        },
        {
          "internalType": "uint256[]",
          "name": "actualAmountsOut",
          "type": "uint256[]"
        }
      ],
      "stateMutability": "view",
      "type": "function"
    },
    {
      "inputs": [
        {
          "internalType": "contract IConfigurableRightsPool",
          "name": "self",
          "type": "IConfigurableRightsPool"
        },
        {
          "internalType": "contract IBPool",
          "name": "bPool",
          "type": "IBPool"
        },
        {
          "internalType": "address",
          "name": "tokenOut",
          "type": "address"
        },
        {
          "internalType": "uint256",
          "name": "tokenAmountOut",
          "type": "uint256"
        },
        {
          "internalType": "uint256",
          "name": "maxPoolAmountIn",
          "type": "uint256"
        }
      ]

typechain fails to recognize the IConfigurableRightsPool type. I would not be so bothered by this normally, but when it fails, it stops the whole compilation.

instead of throwing an exception, this change will assume unknown types are addresses and return them as such.

this is a lot more graceful than killing the whole compile process because some libraries that I don't even use in TS had unknown types.

im not sure if there is a better way to approach this particular issue, but if I am missing something please suggest. For example, still return type address, but throw a warning instead of an error?

instead of throwing an exception, this change will assume unknown types are addresses and return them as such. this is a lot more graceful than killing the whole compile process because some libraries had unknown types.

quezak · 2020-10-29T10:17:09Z

@Killerbyte can you please provide contract code of a minimal reproduction of this issue?

dbeal-eth · 2020-10-29T18:39:46Z

here is the contract which my code fails on:

https://github.com/balancer-labs/configurable-rights-pool/blob/master/libraries/SmartPoolManager.sol

here is the buidler artifact input file:

https://gist.github.com/KillerByte/38ddb41e29295464541e0ece13cd8d3f

if I try building the buidler input file with:

typechain --target=ethers-v5 SmartPoolManager.json --show-stack-traces

I get output

ts-gen: Running TypeChain
ts-gen: artifacts/SmartPoolManager.json matched 1 files.
ts-gen: Processing artifacts/SmartPoolManager.json
Error occured:  Unknown type: IConfigurableRightsPool
Stack trace:  Error: Unknown type: IConfigurableRightsPool
    at Object.parseEvmType (node_modules/typechain/dist/parser/parseEvmType.js:53:11)
    at parseRawAbiParameterType (node_modules/typechain/dist/parser/abiParser.js:135:27)
    at parseRawAbiParameter (node_modules/typechain/dist/parser/abiParser.js:126:15)
    at Array.map (<anonymous>)
    at parseFunctionDeclaration (node_modules/typechain/dist/parser/abiParser.js:117:33)
    at node_modules/typechain/dist/parser/abiParser.js:28:28
    at Array.forEach (<anonymous>)
    at Object.parse (node_modules/typechain/dist/parser/abiParser.js:15:9)
    at Ethers.transformAbiOrFullJsonFile (node_modules/@typechain/ethers-v5/dist/index.js:53:38)
    at Ethers.transformFile (node_modules/@typechain/ethers-v5/dist/index.js:29:21)

dbeal-eth · 2020-10-29T18:40:37Z

@quezak let me know if it is still necessary to isolate the problem further.

quezak · 2020-10-29T19:01:18Z

@Killerbyte I think your proposed solution is too broad -- we don't know what else the solidity compiler might put in the type field, so we may not want to apply this workaround always 😃

I'd update it to return address only if type: "XXX" and internalType: "contract XXX", and if that is also false, throw an error.

@krzkaczor what do you think?

dbeal-eth · 2020-10-29T19:34:44Z

@quezak part of my concern that I think we should explore in this PR is whether or not it is correct to throw an exception and die, or give a "best guess" with a warning, since when I use typechain with buidler, a single failure throws off the whole compilation process, and since I don't control these libraries, it causes problems.

quezak · 2020-10-29T20:01:37Z

@Killerbyte while this may be a good idea in some cases, always guessing address if we don't know the type should be can be wrong in some cases that we're not yet aware of, and we don't want to generate files with invalid typings 😄

IMHO we should apply the fix I mentioned when we have enough hints the type is an address, but keep throwing an error if we really don't know the type. An additional advantage is that someone will report it and we can fix the issue 😃 (and we try to accept simple PRs quickly, so this shouldn't be an issue for developement -- but if you're in a hurry, you can always use patch-package until we do)

If we really wanted to add some workarounds to not fail generation here, the proper way may be to return a special type: "unknown" placeholder, which would result in any being used for the property type -- doesn't stop generation, but also doesn't result in invalid typings, only makes them a bit weaker. Waiting also for @krzkaczor to chip in with his opinion.

krzkaczor · 2020-10-29T21:31:52Z

I basically agree with everything that @quezak said. We already have some similar heuristics in TC codebase. I hate it but what can you do 🤷

To sum up:

If we cannot infer type but type: "XXX" and internalType: "contract XXX" then return address
If we still cannot infer type print out a warning and return any type instead of interrupting the whole process.

@Killerbyte let us know if you want to work on this. We def need tests for this. (2) seems like a breaking change for TypeChain core and all targets unfortunately so I would love to release it together with a fix for #234

dbeal-eth · 2020-10-29T23:37:59Z

sure I will work on it. expect result within a few days.

return "any" type in typescript when unable to parse

dbeal-eth · 2020-11-02T03:40:09Z

@krzkaczor let me know. not sure why circleci is complaining though.

quezak · 2020-11-02T09:15:02Z

@Killerbyte circleci complaints are right 😄 The generated any type is OK, but we also need to handle it in all the target generators. The CI output has all the paths that need fixing -- you can also check the same locally by running yarn test.

krzkaczor · 2020-11-02T11:14:06Z

packages/typechain/src/parser/parseEvmType.ts

@@ -28,6 +31,9 @@ export type TupleType = { type: 'tuple'; components: EvmSymbol[]; originalType:
 // used only for output types
 export type VoidType = { type: 'void' }

+// used when type cannot be detected
+export type UnknownType = { type: 'any'; originalType: string }


I would prefer to keep it consistent: IMO it should be:

export type UnknownType = { type: 'unknown'; originalType: string }

dbeal-eth · 2020-11-02T20:24:00Z

@krzkaczor sorry for the commit spam, was having some computer issues. I'm concerned that additional tests may be necessary for the target libraries to verify that the use of unknown in the functions does not break TS compilation somehow. let me know if you agree that this is the case.

packages/target-ethers-v4/src/codegen/types.ts

krzkaczor · 2020-11-02T22:42:46Z

@Killerbyte it looks good, I left 1 comm.

As for testing it, it would be best if you would add a contract that generates a need to return unknown evm type. This way, we can simply inspect generated output for the new file and treat it as a snapshot rather than real "test". Contracts used for tests are stored in contracts dir.

dbeal-eth · 2020-11-02T22:46:21Z

As for testing it, it would be best if you would add a contract that generates a need to return unknown evm type.

If our goal is to only include this "Unknown" type for ABI inputs/outputs that are not understood by the parseEvmType function, and our goal is for the unknown type to never need to be used, how do we create such a Contract? I bet we could do it if we could manufacture a fake ABI, but any such contracts would probably be slated for fix anyway.

krzkaczor · 2020-11-02T22:51:48Z

Ahh sorry, you're right it shouldn't be possible to construct such a contract. In that case, I think this part is fine. I have a high level of certainty for the code generation part as it's typesafe and basically, compiler forced you to tweak types everywhere.

So two last things:

as mentioned in the previous comment I think that unknown should translate to any
please delete package-lock.json

packages/typechain/src/parser/parseEvmType.ts

also fix formatting

krzkaczor · 2020-11-04T08:54:26Z

Thanks for your hard work @Killerbyte! This looks great! I want to merge a little bit more things before cutting a release so I'll let you know when this is released.

assume type is address if type is unknown

f0579ab

instead of throwing an exception, this change will assume unknown types are addresses and return them as such. this is a lot more graceful than killing the whole compile process because some libraries had unknown types.

quezak requested a review from krzkaczor October 29, 2020 19:01

dbeal-eth added 2 commits November 2, 2020 03:30

realize library contracts

3c2dd66

return "any" type in typescript when unable to parse

add warning log message

dbcb8ba

krzkaczor reviewed Nov 2, 2020

View reviewed changes

dbeal-eth added 4 commits November 2, 2020 17:15

fix circleci errors; rename unknown type

2834722

was missing one of the implementations

04dfcd4

fix prettier

b7cf187

add parse evm type

2a52f61

dbeal-eth force-pushed the patch-1 branch from 9591416 to 2a52f61 Compare November 2, 2020 17:59

krzkaczor reviewed Nov 2, 2020

View reviewed changes

packages/target-ethers-v4/src/codegen/types.ts Show resolved Hide resolved

fix parse evm type

336fa2b

dbeal-eth force-pushed the patch-1 branch from 675d6ab to 336fa2b Compare November 2, 2020 22:54

krzkaczor reviewed Nov 3, 2020

View reviewed changes

packages/typechain/src/parser/parseEvmType.ts Show resolved Hide resolved

dbeal-eth force-pushed the patch-1 branch from 50df137 to fc249d3 Compare November 3, 2020 20:27

krzkaczor reviewed Nov 3, 2020

View reviewed changes

packages/typechain/src/parser/parseEvmType.ts Show resolved Hide resolved

krzkaczor mentioned this pull request Nov 3, 2020

Bump ethers #296

Merged

switch to using any type for unknown variables

f9dc8d0

also fix formatting

dbeal-eth force-pushed the patch-1 branch from fc249d3 to f9dc8d0 Compare November 3, 2020 22:09

krzkaczor approved these changes Nov 4, 2020

View reviewed changes

krzkaczor merged commit 386516f into dethcrypto:master Nov 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

assume type is address if type is unknown #291

assume type is address if type is unknown #291

dbeal-eth commented Oct 28, 2020

quezak commented Oct 29, 2020

dbeal-eth commented Oct 29, 2020

dbeal-eth commented Oct 29, 2020

quezak commented Oct 29, 2020

dbeal-eth commented Oct 29, 2020

quezak commented Oct 29, 2020 •

edited

krzkaczor commented Oct 29, 2020 •

edited

dbeal-eth commented Oct 29, 2020

dbeal-eth commented Nov 2, 2020

quezak commented Nov 2, 2020 •

edited

krzkaczor Nov 2, 2020

dbeal-eth commented Nov 2, 2020

krzkaczor commented Nov 2, 2020

dbeal-eth commented Nov 2, 2020

krzkaczor commented Nov 2, 2020

krzkaczor commented Nov 4, 2020

assume type is address if type is unknown #291

assume type is address if type is unknown #291

Conversation

dbeal-eth commented Oct 28, 2020

quezak commented Oct 29, 2020

dbeal-eth commented Oct 29, 2020

dbeal-eth commented Oct 29, 2020

quezak commented Oct 29, 2020

dbeal-eth commented Oct 29, 2020

quezak commented Oct 29, 2020 • edited

krzkaczor commented Oct 29, 2020 • edited

dbeal-eth commented Oct 29, 2020

dbeal-eth commented Nov 2, 2020

quezak commented Nov 2, 2020 • edited

krzkaczor Nov 2, 2020

Choose a reason for hiding this comment

dbeal-eth commented Nov 2, 2020

krzkaczor commented Nov 2, 2020

dbeal-eth commented Nov 2, 2020

krzkaczor commented Nov 2, 2020

krzkaczor commented Nov 4, 2020

quezak commented Oct 29, 2020 •

edited

krzkaczor commented Oct 29, 2020 •

edited

quezak commented Nov 2, 2020 •

edited