feat: ABI parser #3089
feat: ABI parser #3089
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
…nto ns/feat/abi-parser
CodSpeed Performance ReportMerging #3089 will not alter performanceComparing Summary
|
| public toAbiType(): AbiType { | ||
| return { | ||
| swayType: this.type, | ||
| concreteTypeId: this.typeId as string, |
There was a problem hiding this comment.
Why do we cast this as a string?
Will there be instances that the concreteTypeId can be a metadataTypeId?
| export interface AbiComponentV1 extends AbiTypeArgumentV1 { | ||
| readonly name: string; | ||
| } | ||
|
|
||
| export interface AbiTypeArgumentV1 { | ||
| readonly typeId: number | string; // the type metadata declaration ID or type concrete declaration hash based ID of the type of the component. | ||
| readonly typeArguments?: readonly AbiTypeArgumentV1[]; | ||
| } |
There was a problem hiding this comment.
I'm going to unresolve this one and agree with @Torres-ssf suggestion:
As forc generates the ABI with the name in the typeArguments - I believe we should match this behaviour.
cc @nedsalk
| @@ -0,0 +1,90 @@ | |||
| export interface Abi { | |||
| specVersion: string; | |||
There was a problem hiding this comment.
As this ABI is "normalized" and not to spec, do we want to make the specVersion reflect this?
| specVersion: string; | |
| specVersion: 'normalized'; |
|
|
||
| export class ResolvableType { | ||
| private metadataType: AbiMetadataTypeV1; | ||
| type: string; |
There was a problem hiding this comment.
| type: string; | |
| swayType: string; |
Suggest changing this variable to be consistent across the codebase.
| encodingVersion: abi.encodingVersion, | ||
| programType: abi.programType, | ||
| functions: abi.functions.map((fn: AbiFunctionV1) => ({ | ||
| attributes: fn.attributes?.map(mapAttribute) ?? undefined, |
There was a problem hiding this comment.
I suppose it could throw - providing the attributes are malformed.
…nto ns/feat/abi-parser
There was a problem hiding this comment.
First pass. I am still getting familiarized with the code and going through the review process.
The code contains complex algorithms with minimal comments, making it challenging to understand or modify.
To improve clarity and maintainability, we should consider adding detailed comments to explain each case and include TSDoc for the class methods.
Could you confirm if my understanding of these terms is accurate?
-
ResolvableType: Represents a type that may contain generics or unresolved components. -
ResolvedType: Represents a fully resolved type where all generics and type parameters have respective concrete types.
Should we add tests for the new class ResolvableType?
|
|
||
| const resolvedGenericType = typeArgs?.find( | ||
| ([tp]) => (c.type as ResolvableType).metadataTypeId === tp | ||
| )?.[1]; |
There was a problem hiding this comment.
Why use .[1];? We need to properly document this.
| private resolveInternal( | ||
| typeId: string | number, | ||
| typeParamsArgsMap: Array<[number, ResolvedType]> | undefined | ||
| ): ResolvedType { |
There was a problem hiding this comment.
This method contains complex logic and appears to address multiple concerns. Would it be possible to refactor it into smaller helper functions, each with a single responsibility?
Even if these helpers are only used once, giving them descriptive names can greatly enhance the code's readability and make its purpose easier to understand.
There was a problem hiding this comment.
Yup +1 here, can appreciate the logic is there but just breaking the code down more will really benefit resolvable-type.ts/resolved-type.ts.
|
|
||
| return ( | ||
| resolved ?? [ | ||
| +typeParameter, |
There was a problem hiding this comment.
Why are you using a + here? Could typeParameter be null or a string instead of a number? It’s worth documenting this for clarity.
| if (swayTypeMatchers.generic(mt.type)) { | ||
| const resolvedTypeParameter = parent.typeParamsArgsMap?.find( | ||
| ([tp]) => tp === mt.metadataTypeId | ||
| )?.[1]; |
There was a problem hiding this comment.
Why .[1]; ? We need to document all these edge cases properly.
| abi: AbiSpecificationV1, | ||
| type: AbiConcreteTypeV1 | ||
| ): ResolvedType { | ||
| if (type.metadataTypeId === undefined) { |
There was a problem hiding this comment.
Why explicitly compare this to equals to undefined?
There was a problem hiding this comment.
Some utility functions used across the code for isConcreteType and isMetadataType that checks the condition I think would make this more readable.
| return metadataType.typeParameters?.map((typeParameter, idx) => [typeParameter, args[idx]]); | ||
| } | ||
|
|
||
| private static handleComponent( |
There was a problem hiding this comment.
I believe we can rename this method to createResolvableComponent as it reflects its purpose clearly
…adataType` simplifications
…nto ns/feat/abi-parser
| resolveableType.type !== 'struct std::vec::RawVec' && | ||
| resolveableType.type !== 'struct std::bytes::RawBytes' |
There was a problem hiding this comment.
What's the reasoning for these filters? Are these std types filtered anywhere else? In the current version, we have various exceptions littered around, would be good to have these centralised.
| /** | ||
| * Vectors consist of multiple components, | ||
| * but we only care about the `buf`'s first type argument | ||
| * which defines the type of the vector data. | ||
| * Everything else is being ignored, | ||
| * as it's then easier to reason about the vector | ||
| * (you just treat is as a regular struct). | ||
| */ | ||
| if (swayTypeMatchers.vector(this.metadataType.type)) { | ||
| components = components?.map((component) => { | ||
| if (component.name === 'buf') { | ||
| return component.typeArguments?.[0]; | ||
| } | ||
| return component; | ||
| }) as AbiComponentV1[]; | ||
| } |
There was a problem hiding this comment.
Could this not be handled at the vec component level rather than the constructor of the resolvable type for a better separation of concerns, I think this file should be type agnostic.
| }; | ||
| } | ||
|
|
||
| constructor( |
There was a problem hiding this comment.
Nit, please can the constructor be the first public method.
| ): ResolvableComponent { | ||
| const name = (component as AbiComponentV1).name; | ||
|
|
||
| if (typeof component.typeId === 'string') { |
There was a problem hiding this comment.
Please can this go in a utility/helper function for isContectyType, typeId being a string is an odd classification and I think that'll bring clarity to the code.
| abi: AbiSpecificationV1, | ||
| type: AbiConcreteTypeV1 | ||
| ): ResolvedType { | ||
| if (type.metadataTypeId === undefined) { |
There was a problem hiding this comment.
Some utility functions used across the code for isConcreteType and isMetadataType that checks the condition I think would make this more readable.
| }); | ||
| } | ||
|
|
||
| if (!type.typeArguments) { |
There was a problem hiding this comment.
Again, why does this mean we need to resolve the internal type?
| * in which case they cannot be resolved. | ||
| * If they're not generic, they can be immediately resolved. | ||
| */ | ||
| private handleMetadataType( |
There was a problem hiding this comment.
As mentioned here, I think this function could also benefit from some docs above the conditions, just denoting why certain things have to happen at certain points.
| @@ -0,0 +1,90 @@ | |||
| export interface Abi { | |||
There was a problem hiding this comment.
Is this the parsed types? And ../specifications/specification.ts contains the spec types? If so I'd be pro removing the ABI prefix and replacing/adding a Parsed prefix.
| private resolveInternal( | ||
| typeId: string | number, | ||
| typeParamsArgsMap: Array<[number, ResolvedType]> | undefined | ||
| ): ResolvedType { |
There was a problem hiding this comment.
Yup +1 here, can appreciate the logic is there but just breaking the code down more will really benefit resolvable-type.ts/resolved-type.ts.
Summary
Adds the parsing functionality for an ABI. The
Abiinterface outputted byAbiParserwill be the (hopefully) unchanging interface that will be used both by us in coders and typers, as well as external users who wish to work with the abi but not be affected by changing specifications.This PR doesn't have any tests as the outputs of
AbiParser.parseare tested in #3249 and #3402 which are based off of this PR.Checklist