Abi integration #466
Abi integration #466
Conversation
packages/abi/src/abi-method.ts
Outdated
|
|
||
| export type ABIMethodReturn = { | ||
| type: ABIReturnType | ||
| type: ABIType | 'void' |
There was a problem hiding this comment.
| type: ABIType | 'void' | |
| type: ABIMethodReturnType |
| * | ||
| * This value will be undefined if decoding failed or the method does not return a value (return type "void") | ||
| */ | ||
| returnValue: ABIValue |
There was a problem hiding this comment.
Should this be optional?
packages/abi/src/abi-method.ts
Outdated
| method: ABIMethod | ||
| /** The raw return value as bytes. | ||
| * | ||
| * This value will be empty if the method does not return a value (return type "void") |
There was a problem hiding this comment.
This value will be empty
As in an empty Uint8Array, rather than undefined?
There was a problem hiding this comment.
I missed the comments in this type when integrating ABI features into utils. I think empty and undefined aren't applicable anymore.
- The method returns
undefinedwhen ABIMethod returnsvoid. It doesn't returnABIReturn. - There is another union variant that describes the decoding error case.
I updated the comments to reflex this fact.
There was a problem hiding this comment.
Does this need to go into the migration guide or was it previous behaviour too?
There was a problem hiding this comment.
It's a previous behaviour
algokit-utils-ts/src/types/app-manager.ts
Line 432 in 3d899a4
and the type for
ABIReturn is
/** The return value of an ABI method call */
export type ABIReturn =
| {
rawReturnValue: Uint8Array
returnValue: ABIValue
method: ABIMethod
decodeError: undefined
}
| { rawReturnValue?: undefined; returnValue?: undefined; method?: undefined; decodeError: Error }
packages/abi/src/abi-method.ts
Outdated
| } | ||
|
|
||
| const name = signature.slice(0, argsStart) | ||
| const args = parseTupleContent(signature.slice(argsStart + 1, argsEnd)) // hmmm the error is bad |
There was a problem hiding this comment.
// hmmm the error is bad
?
There was a problem hiding this comment.
This comment was copied over from algokit-core. I'm not sure the context now to be honest. I had a look at the error messages inside parseTupleContent and they look ok to me.
One thing jumped out is that in abi-type.ts we use prefix for error messages, for example Validation Error: the content should not have consecutive commas. The rest of utils-ts doesn't do that.
There was a problem hiding this comment.
I think we keep consistent with utils, so remove the prefix. I guess remove the comment too?
| bare_call_config: CallConfig | ||
| } | ||
|
|
||
| interface ABIContractParams { |
There was a problem hiding this comment.
Not sure ABI is the correct term to use here?
ABI mainly refers to the encoding standard. Maybe AppSpecParams? Same applies for ABIContractNetworks and ABIContractNetworkInfo, however the rest of the names seem ok to me.
Potentially this also allows us to drop the Param suffix?
There was a problem hiding this comment.
The names were copied from algosdk over. I think ContractDefinition is a better name. What do you think?
There was a problem hiding this comment.
Oh right, maybe it's fine to keep the same. Are they user facing types?
There was a problem hiding this comment.
I'll revert to the algosdk names. They aren't explicitly exported by are referenced by AppSpec which is public
packages/abi/src/abi-type.ts
Outdated
| if (str.endsWith(']')) { | ||
| const stringMatches = str.match(STATIC_ARRAY_REGEX) | ||
| if (!stringMatches || stringMatches.length !== 3) { | ||
| throw new Error(`malformed static array string: ${str}`) |
There was a problem hiding this comment.
| throw new Error(`malformed static array string: ${str}`) | |
| throw new Error(`Malformed static array string: ${str}`) |
We tend to start error message with uppercase. This applies to all the changes removing the prefix.
Proposed Changes