Add struct parsing to @typechain/web3-v1 (#779) #801
Conversation
🦋 Changeset detectedLatest commit: a2e307c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@mihai9-lab there are still some problems with tests. Can u take a look? |
There was a problem hiding this comment.
Excited to see this being worked on! Commenting a few nits/flagging a bug we ran into trying to use @mihai9-lab's target
| } else { | ||
| return `${item}[]` | ||
| } |
There was a problem hiding this comment.
Hi @mihai9-lab, we ran across a bug on this line specifically. In the case where item is a string declaring a union type (for example: string | number | BN), just appending [] to this type results in string | number | BN[] instead of the intended type (string | number | BN)[] or perhaps string [] | number[] | BN[] (I prefer the first solution, as it allows mixing of types in input arrays).
I was able to fix this with the following code:
if (item.contains("|")) {
item = `(${item})`
}
However, it may be cleaner to parametrize this function with another boolean and determine whether item is a union upstream of this function, and/or accept length and this proposed boolean as properties of an options object.
There was a problem hiding this comment.
Seems like the original code here just wrapped all types in parenthesis regardless of whether the type was simple or a union type. I leave the exact form of the solution up to you!
| data: [string, string]; | ||
| 0: [string, string]; | ||
| data: Events.EventDataStructOutput; | ||
| 0: Events.EventDataStructOutput; |
There was a problem hiding this comment.
nit: I prefer keeping these types flat for ease of use during development (would prefer to see that the type is a [string, string] instead of a Events.EventDataStructOutput when hovering over it, for example)
| ${outputs.map((t) => t.name && `${t.name}: ${codegenOutputType({ useStructs: true }, t.type)}, `).join('')} | ||
| ${outputs.map((t, i) => `${i}: ${codegenOutputType({ useStructs: true }, t.type)}`).join(', ')} |
There was a problem hiding this comment.
any chance we can reverse this function's type params to codegenOutputType(type, options) instead of codegenOutputType(options, type)? The first is more likely to not break code, since it doesn't change the position of the type param.
There was a problem hiding this comment.
Seems like most codegen functions that take in an options param also take it as the last parameter
| useStructs?: boolean // uses struct type for first depth, if false then generates first depth tuple types | ||
| } | ||
|
|
||
| export function codegenInputTypes(options: GenerateTypeOptions, input: Array<AbiParameter>): string { |
There was a problem hiding this comment.
Same feedback for codegenInputTypes (prefer codegenInputTypes(input, options?))
|
@mihai9-lab when do you think you'll have bandwidth to work on this change? Want to get this through pretty urgently, can I work on this now if you don't have time soon? |
Hey, sorry for not replying sooner. Currently I'm pretty busy and won't be able to work on this for at least a week or two. |
|
The approach of this PR might be a bit flawed: as seen here: https://web3js.readthedocs.io/en/v1.8.1/web3-eth-contract.html#methods-mymethod-call "Promise returns Mixed: The return value(s) of the smart contract method. If it returns a single value, it’s returned as is. If it has multiple return values they are returned as an object with properties and indices" Since the return is "an object with properties and indices", we cannot return the type of a struct as a union of an array and an object, as this will annotate the object with all the members of an array (for example, .concat), which do not actually exist on the object. |
|
Yeah i didn't expect it to behave like that, it seems that typescript does assume object has all properties of an array(although it isn't one). I see 2 possible approaches to deal with this - one being to change array generation to object generation, other being to leave it as is but add an additional "converter" type which would exclude unwanted array properties. I made a quick array to objected converter type as a proof of concept if going with second option, here's a playground for that. Maybe this is a bit hacky, so i'd like to see what you and the community thinks is the best approach. |
No description provided.