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

feat: add support for numbers in multicall #2612

Merged
merged 14 commits into from
Nov 4, 2024
Merged

Conversation

kfastov
Copy link
Contributor

@kfastov kfastov commented Oct 25, 2024

Closes #2245

Introduced changes

  • Added new enum type Input with variants String and Number(i64) to support both string and numeric inputs in TOML files
  • Updated DeployCall and InvokeCall structs to use Vec<Input> instead of Vec<String> for their inputs field
  • Modified parse_inputs function to handle both string and numeric input variants
  • Added #[serde(untagged)] attribute to enable seamless deserialization of both input types from TOML

Checklist

  • Linked relevant issue
  • Updated relevant documentation
  • Added relevant tests
  • Performed self-review of the code
  • Added changes to CHANGELOG.md

Other notes

  • Numbers larger than 2^64 are not supported by TOML format: https://toml.io/en/v1.0.0#integer
  • However, numbers outside i64 range cannot be supported currently due to an issue in the toml parser

@kfastov kfastov marked this pull request as ready for review October 25, 2024 20:11
@Arcticae
Copy link
Contributor

@kfastov can you please also test the case where the number exceeds the limit (so we make sure it fails with a reasonable message)?

@Arcticae
Copy link
Contributor

@kfastov bump

@Arcticae
Copy link
Contributor

@kfastov are you still working on this? Please let me know so i can reassign if you're not :)

@kfastov
Copy link
Contributor Author

kfastov commented Oct 30, 2024

Hi @Arcticae
Sorry for delayed reply, I had a multi-day transfer with no internet, I am ready to finish this PR now, and I'll do it today or tomorrow :)

@Arcticae
Copy link
Contributor

Thx for the update 👍

Copy link
Contributor

@Arcticae Arcticae left a comment

Choose a reason for hiding this comment

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

cc @cptartur can you assign someone to take a second look?

Copy link
Collaborator

@franciszekjob franciszekjob left a comment

Choose a reason for hiding this comment

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

Left only two nits, looking good 👍

docs/src/starknet/multicall.md Outdated Show resolved Hide resolved
docs/src/starknet/multicall.md Outdated Show resolved Hide resolved
Wrap number and string literals and data types as suggested by @franciszekjob

Co-authored-by: Franciszek Job <[email protected]>
@kfastov
Copy link
Contributor Author

kfastov commented Oct 31, 2024

@franciszekjob Addressed the nits, please check

Copy link
Collaborator

@franciszekjob franciszekjob left a comment

Choose a reason for hiding this comment

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

LGTM

@cptartur cptartur removed their request for review October 31, 2024 16:24
@Arcticae Arcticae added this pull request to the merge queue Nov 4, 2024
@Arcticae
Copy link
Contributor

Arcticae commented Nov 4, 2024

@kfastov thx for your contribution!

Merged via the queue into foundry-rs:master with commit 0e0d7e5 Nov 4, 2024
25 checks passed
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.

Add support for numbers in multicall
3 participants