-
Notifications
You must be signed in to change notification settings - Fork 9
fix: enum conversion of primitive types would invalidate args #83
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
base: main
Are you sure you want to change the base?
Conversation
|
I took the opportunity to also check the latest changes to the same utils for type conversions on laboratory and updated this piece as these weren't altering much of the original behavior but extending some additional checks. |
|
I also opened an issue on Laboratory as I discussed with Jeesun: stellar/laboratory#1502 The fix in this PR should cover most cases, but I figure they might implement a broader fix if there could be more complex and rarer edge cases. Therefore, we can monitor and revisit this fix if necessary. |
pselle
left a 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.
Looks good! Non-blocking questions, you can change or not.
| const getScValFromPrimitive = (v: PrimitiveArg) => { | ||
| if (v.type === "bool") { | ||
| const boolValue = v.value === "true"; | ||
| const boolValue = v.value === "true" ? true : false; |
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.
Do you gain anything from adding this ternary? I wouldn't block on asking it to be removed, but I'm curious.
| }; | ||
|
|
||
| const getScValsFromArgs = ( | ||
| export const getScValsFromArgs = ( |
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.
Does this need to be exported?
| const boolValue = v.value === "true"; | ||
| const boolValue = v.value === "true" ? true : false; |
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.
These lines should be equivalent.
| if ( | ||
| typeof argValue === "object" && | ||
| argValue && | ||
| "type" in argValue && | ||
| "value" in argValue && | ||
| argValue.type && | ||
| argValue.value | ||
| ) { | ||
| if ((argValue as AnyObject).type && (argValue as AnyObject).value) { |
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.
If it all works, great, but I'm wary of all these changes from type guards to casting. Normally, I try to avoid using the as keyword as much as possible because it's overriding type checking. Runtime errors can easily crop up when the values aren't what we expect.
|
@fazzatti Did you want to move this PR forward? |
|
Reverting to draft to wait on stellar/laboratory#1502 first |
Issue:
When using args combination involving primitive types and enums together, the enum logic conversion for the arg values would wrongly try to covert the primitive values, invalidating those and subsequently the transaction invocation.
E.g.: given a function with payload:
The conversion would generate an invocation with an invalid void arg for the Address.
Solution:
I've extended on the original laboratory logic by adding another condition before falling into the last default option, to identify types other than enum and to process those as primitive types.