Skip to content
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

Add support for nativeToScVal to convert strings to all number sizes. #763

Merged
merged 5 commits into from
Jul 19, 2024

Conversation

Shaptic
Copy link
Contributor

@Shaptic Shaptic commented Jul 18, 2024

Previously, i32 and u32 could not automatically come from string values when using nativeToScVal. Namely, nativeToScVal("4", { type: "i32" }) was explicitly forbidden. This was my design: the rationale was that if it's a 32-bit integer, it should be passed as a Number.

However, this logic does not hold true for maps, because in JavaScript, all map keys are strings. Therefore, creating an ScVal representing an integer-to-integer map is impossible via nativeToScVal, e.g., a function signature like this one:

fn spin(env: Env, pairs: Map<u32, u64>) -> boolean;

cannot be executed by

nativeToScVal(
  { 1234: 5678 }, 
  { 
    type: { 
      "1234": [ "u32, "u64" ],
      "5678": [ "u32, "u64" ],
    }
);

because 1234 would be interpreted as a string, and thus could not be automatically converted to a number for a u32 inference.

Copy link

Size Change: +1.48 kB (+0.05%)

Total Size: 3.21 MB

Filename Size Change
dist/stellar-base.js 2.36 MB +1.08 kB (+0.05%)
dist/stellar-base.min.js 857 kB +400 B (+0.05%)

compressed-size-action

@Shaptic Shaptic requested review from sreuland, psheth9, a team, chadoh and willemneal July 18, 2024 22:58
@Shaptic Shaptic self-assigned this Jul 18, 2024
@Shaptic Shaptic added the bug label Jul 18, 2024
expect(parseInt(newTx.operations[1].amount)).to.equal(2);
}),
1
);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewers' note: this is just whitespace changes from a stray yarn fmt (not sure how the unformatted versions snuck in)

@Shaptic Shaptic mentioned this pull request Jul 18, 2024
@Shaptic Shaptic added the rpc-sdk-scrum Align with Platform's RPC/SDK planning board label Jul 18, 2024
Shaptic added a commit that referenced this pull request Jul 18, 2024
Copy link
Contributor

@sreuland sreuland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

detailed pr description was very helpful for doing review, thanks!

Copy link
Member

@willemneal willemneal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug rpc-sdk-scrum Align with Platform's RPC/SDK planning board
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants