Skip to content

Conversation

@fazzatti
Copy link
Contributor

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:

    pub fn example_fn(
        _e: &Env,
        provider: Address,        // <-- combining an Address arg
        status: CustomStatus, // <-- with a enum arg
    ) 
// do anything
    }

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.

@fazzatti fazzatti self-assigned this Jul 14, 2025
@fazzatti
Copy link
Contributor Author

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.

@fazzatti fazzatti requested review from pselle and zachfedor July 14, 2025 23:38
@fazzatti fazzatti changed the title fix: enum conversion of primitive types would invalid args fix: enum conversion of primitive types would invalidate args Jul 15, 2025
@fazzatti
Copy link
Contributor Author

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.

Copy link
Member

@pselle pselle left a 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;
Copy link
Member

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 = (
Copy link
Member

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?

Comment on lines -438 to +467
const boolValue = v.value === "true";
const boolValue = v.value === "true" ? true : false;
Copy link
Contributor

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.

Comment on lines -550 to +582
if (
typeof argValue === "object" &&
argValue &&
"type" in argValue &&
"value" in argValue &&
argValue.type &&
argValue.value
) {
if ((argValue as AnyObject).type && (argValue as AnyObject).value) {
Copy link
Contributor

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.

@pselle
Copy link
Member

pselle commented Jul 30, 2025

@fazzatti Did you want to move this PR forward?

@fazzatti fazzatti marked this pull request as draft August 4, 2025 16:55
@fazzatti
Copy link
Contributor Author

fazzatti commented Aug 4, 2025

Reverting to draft to wait on stellar/laboratory#1502 first

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants