Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default solidity JSON ABI type not supported by Contract or Interface #602

Closed
andrevmatos opened this issue Sep 8, 2019 · 17 comments
Closed
Labels
bug Verified to be an issue. fixed/complete This Bug is fixed or Enhancement is complete and published.

Comments

@andrevmatos
Copy link

I'm creating contract instances by importing the JSON ABI (out of solidity, or as copied from etherscan contract page e.g.) in typescript and passing it to the constructor, but TypeScript complains the JSON (fully-declared) type can't be assigned to contractInterface: Array<string | ParamType> | string | Interface.
Example

import TokenAbi from './contracts/Token.json';
const contract = new Contract(tokenAddress, TokenAbi, signer);

Funny enough, a workaround seems to be just casting it as one of the union types, as the Interface constructor can handle the provided fragment array, but it's wrong.

The problem I see is that ParamType[] (the part of the union I suppose was expected to relate to this kind of use) isn't a valid ABI, as it'd represent just the type of a input or output of either a function or event. Instead, I'd say the correct interface for the Contract constructor would be something like:

contractInterface: Array<string> | Array<FunctionFragment | EventFragment> | string | Interface

This change could be reflected in the Interface constructor type as well.

Additionally, I believe the FunctionFragment.gas member should be made optional (?), as it isn't present in the JSON abi fragments.

@ricmoo
Copy link
Member

ricmoo commented Sep 8, 2019

So, it is important to note that your import from a JSON filename is not importing the value as JSON. It performs a JSON.parse, so it is actually an object. JSON is the string representation. So, if you pass in the JSON.stringify(TokenAbi), it will work fine, since it is a string, which is an allowed type. :)

It’s hard to accept a generic object in a type-safe way. I’ll look into expanding the fragment types, as this was something I wrote quite a while ago when still learning TypeScript.

@andrevmatos
Copy link
Author

Right, when I wrote JSON (fully-declared) type I was not precise, as I was trying to refer to the object type. The good part about is that, when importing a JSON file directly (with resolveJsonModule: true in tsconfig), TypeScript actually does a pretty good job in infering the resulting object's schema/type. So it wouldn't be a generic object, but instead a complete schema which can be made compatible with your current fragments with the types I mentioned, which could help on this. Doing a stringify would actually be more or less the same as casting it to the one of the supported types, as Interface's constructor would immediatelly parse it back and proceed as with the casted object. Fortunately, the Fragments types are already close enough (minus only that gas member) to the parsed ABI type, so it should be feasible to have proper typing for this usecase.
Thank you very much for your awesome work on this. It's a true pleasure to work with this library on https://github.com/raiden-network/light-client =)

@ricmoo
Copy link
Member

ricmoo commented Sep 8, 2019

(oh, and yes, gas should certainly be an optional property; only Vyper and the Human-Readable ABI format have wide support for it)

@ricmoo ricmoo added the discussion Questions, feedback and general information. label Sep 8, 2019
@ricmoo
Copy link
Member

ricmoo commented Jan 7, 2020

In your above example, can you try using TokenAbi.abi (instead of just TokenAbi)?

Looking more into that, I think that may be the issue. I have confirmed in v4 that gas is not verified at the TypeScript compiler level...

@andrevmatos
Copy link
Author

On the JSON I mentioned, there's no abi, it already contains the ABI (it's a list of the fragments, as I mentioned), but because of that simple type mismatch, it isn't accepted. I'll try to prepare a PR with tests to implement it.
About gas, I understand it isn't verified nor expect to, just the property being required makes it incompatible with the common definition I mentioned, therefore it should be made optional.

@ricmoo
Copy link
Member

ricmoo commented Jan 11, 2020

Re-reading your original comment with inspecting the source more, I think it is a bug.

Looking at the Contract and Interface objects, it looks like I'm expecting an Array<string | ParamType>, but I think that should be Array<string | FunctionFragment | EventFragment>, which is what you were saying in the first place. I'm trying to figure out why I did that. I think early on before I have a "Fragment" type I was using "Param", and there was an issue with the refactor (renaming Param to ParamType instead of Fragment...

I just need to make sure this won't break anyone if I make the change, but if anyone passes in a param type, it will get cast to a fragment and fall apart in other unfortunate ways, so I think the change is safe. Testing right now...

ricmoo added a commit that referenced this issue Jan 11, 2020
@ricmoo ricmoo added bug Verified to be an issue. on-deck This Enhancement or Bug is currently being worked on. and removed discussion Questions, feedback and general information. labels Jan 11, 2020
@ricmoo
Copy link
Member

ricmoo commented Jan 11, 2020

I've made the changes. Can you try out 4.0.43 and let me know if there are any other problems?

Thanks! :)

@ricmoo ricmoo added fixed/complete This Bug is fixed or Enhancement is complete and published. and removed on-deck This Enhancement or Bug is currently being worked on. labels Jan 11, 2020
@ksvirsky
Copy link

4.0.41:
TS2345: Argument of type '({ "inputs": { "internalType": string; "name": string; "type": string; }[]; "payable": boolean; "stateMutability": string; "type": string; } | { "anonymous": boolean; "inputs": { "indexed": boolean; "internalType": string; "name": string; "type": string; }[]; "name": string; "type": string; } | { ...; })[]' is not assignable to parameter of type 'string | (string | ParamType)[] | Interface'.
Type '({ "inputs": { "internalType": string; "name": string; "type": string; }[]; "payable": boolean; "stateMutability": string; "type": string; } | { "anonymous": boolean; "inputs": { "indexed": boolean; "internalType": string; "name": string; "type": string; }[]; "name": string; "type": string; } | { ...; })[]' is not assignable to type '(string | ParamType)[]'.

4.0.43:
TS2345: Argument of type '({ "inputs": { "internalType": string; "name": string; "type": string; }[]; "payable": boolean; "stateMutability": string; "type": string; } | { "anonymous": boolean; "inputs": { "indexed": boolean; "internalType": string; "name": string; "type": string; }[]; "name": string; "type": string; } | { ...; })[]' is not assignable to parameter of type 'string | (string | FunctionFragment | EventFragment)[] | Interface'.
Type '({ "inputs": { "internalType": string; "name": string; "type": string; }[]; "payable": boolean; "stateMutability": string; "type": string; } | { "anonymous": boolean; "inputs": { "indexed": boolean; "internalType": string; "name": string; "type": string; }[]; "name": string; "type": string; } | { ...; })[]' is not assignable to type '(string | FunctionFragment | EventFragment)[]'.
and
TS2345: Argument of type '({ "inputs": { "internalType": string; "name": string; "type": string; }[]; "payable": boolean; "stateMutability": string; "type": string; } | { "anonymous": boolean; "inputs": { "indexed": boolean; "internalType": string; "name": string; "type": string; }[]; "name": string; "type": string; } | { ...; } | { ...; })[]' is not assignable to parameter of type 'string | (string | FunctionFragment | EventFragment)[] | Interface'.
Type '({ "inputs": { "internalType": string; "name": string; "type": string; }[]; "payable": boolean; "stateMutability": string; "type": string; } | { "anonymous": boolean; "inputs": { "indexed": boolean; "internalType": string; "name": string; "type": string; }[]; "name": string; "type": string; } | { ...; } | { ...; })[]' is not assignable to type '(string | FunctionFragment | EventFragment)[]'.
Type '{ "inputs": { "internalType": string; "name": string; "type": string; }[]; "payable": boolean; "stateMutability": string; "type": string; } | { "anonymous": boolean; "inputs": { "indexed": boolean; "internalType": string; "name": string; "type": string; }[]; "name": string; "type": string; } | { ...; } | { ...; }' is not assignable to type 'string | FunctionFragment | EventFragment'.
Type '{ "inputs": { "internalType": string; "name": string; "type": string; }[]; "payable": boolean; "stateMutability": string; "type": string; }' is not assignable to type 'string | FunctionFragment | EventFragment'.
Type '{ "inputs": { "internalType": string; "name": string; "type": string; }[]; "payable": boolean; "stateMutability": string; "type": string; }' is missing the following properties from type 'FunctionFragment': name, constant, outputs

@andrevmatos
Copy link
Author

andrevmatos commented Jan 22, 2020

Yes, this is failing in our repo too:
raiden-network/light-client#892 (comment)
Seems like fallback method doesn't comply with FunctionFragment. It may be needed to either add a FallbackFrament to the array union, or make FunctionFragment itself a union between current schema and the fallback schema ({ payable: boolean; stateMutability: string; type: "fallback"; })

@ricmoo
Copy link
Member

ricmoo commented Jan 22, 2020

Oh, that is interesting...

The library drops unknown types, but TS is a bit pickier, so I’ll add something for this Type. It won’t have its own fragment type in v4, but I’ll get TS to behave itself. And I’ll make sure v5 understands too.

What compiler version is this? I know 0.6 has added some new types, and in need to check on what the ABI puts in place of those new pseudo-functions...

@snario
Copy link

snario commented Jan 29, 2020

I am using 0.5.13+commit.5b0b510c.Darwin.appleclang and see this.

@andrevmatos
Copy link
Author

@ricmoo I believe we used solidity 0.5.4+commit.9549d8ff to get these fallback methods

@ricmoo
Copy link
Member

ricmoo commented Jan 29, 2020

by the way, the rollback to include ParamType can be tracked here:

#721

@ricmoo
Copy link
Member

ricmoo commented Jan 30, 2020

I think this ticket is resolved now, no? I'm going to close it, but if it is still an issue, please re-open.

I've also published the fix for #721, but will leave that open a bit longer to make sure that problem is addressed too.

Thanks! :)

@ricmoo ricmoo closed this as completed Jan 30, 2020
@ricmoo
Copy link
Member

ricmoo commented Jan 30, 2020

(also thanks for the Solidity version, I'll check out the generated ABI for that too)

@andrevmatos
Copy link
Author

Yes, indeed the current version works for our usecase: the fallback function, of type { payable: boolean; stateMutability: string; type: string } is assignable to union member ParamType, which is { name?: string; type: string; indexed?: boolean: components?: Array<any> }, so the inferred abi type is assignable to the constructor parameter without casting.
Thanks!

@ricmoo
Copy link
Member

ricmoo commented Jan 31, 2020

Awesome! Glad to hear it. Yes, basically the ParamType is very forgiving, only requiring a "type", which is why it worked before. When I removed it, I was thinking that no one would pass in a ParamType to that function, but now I realize that people were passing things in that shared its shape. :)

In v5, this is replaced by the JsonFragment interface, which matches the types of things that Solidity outputs in its ABI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified to be an issue. fixed/complete This Bug is fixed or Enhancement is complete and published.
Projects
None yet
Development

No branches or pull requests

4 participants