-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat: typegen support for passing Uint8Array
as Vec<u8>
#2884
Conversation
Coverage Report:
Changed Files:Coverage values did not change👌. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const INPUT = [8, 6, 7, 5, 3, 0, 9]; | ||
const INPUT_AS_UINT8_ARRAY = Uint8Array.from(INPUT); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const INPUT = [8, 6, 7, 5, 3, 0, 9]; | |
const INPUT_AS_UINT8_ARRAY = Uint8Array.from(INPUT); | |
const INPUT: Vec<BigNumberish> = [8, 6, 7, 5, 3, 0, 9]; | |
const INPUT_AS_UINT8_ARRAY: Vec<BigNumberish> = Uint8Array.from(INPUT); |
Is it worth adding the types here, to ensure they work as intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petertonysmith94 there's a separate issue for those types #2785, but I'm gonna be making changes to this PR regardless: #2884 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handling the problem on the Vec
common type level won't work because it'll affect other types, as well as outputs, which shouldn't be affected. This problem will have to be solved in a different manner, e.g. on the VectorType
level or somewhere - I'm not sure where.
@@ -14,10 +16,10 @@ export type Enum<T> = { | |||
*/ | |||
export type Option<T> = T | undefined; | |||
|
|||
export type Vec<T> = T[]; | |||
export type Vec<T> = T extends BigNumberish ? T[] | Uint8Array : T[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be a problem for u16
and u32
values because BigNumberish
is used for them as well, but the Uint8Array
cannot store their values and that is the reason why I created #2785. Users might get the false impression that they can pass Uint8Array
for u16
and u32
vectors.
I think that this problem cannot be solved on the level of the generated types like this; rather, the checks must be done somewhere in typegen where abi type parsing is happening.
const { waitForResult } = await contractInstance.functions | ||
.echo_u8(INPUT_AS_UINT8_ARRAY) | ||
.call<Uint8Array>(); | ||
const { value } = await waitForResult(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are always returning a number[]
here in runtime so this should never be Uint8Array
. As mentioned above, this will have to be handled differently because the Vec
type is shared between all vectors, both input and output ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nedsalk that's a great catch. I completely missed this.
I considered this deeper approach with changes deeper inside the Vec type but thought I might be able to get away with something quick like this. I will revert this PR and come back with a better approach that is more modular and flexible
Closing since #1553 is not planned for now |
Release notes
In this release, we:
Uint8Array
asVec<u8
Summary
This PR improves typegen support for the
u8
type by addingUint8Array
as a valid input type on the function signature alongsideVec<BigNumberish>
.Checklist
tests
to prove my changesdocs