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

Sui JSON support optional input #7118

Merged
merged 4 commits into from
Jan 11, 2023
Merged

Sui JSON support optional input #7118

merged 4 commits into from
Jan 11, 2023

Conversation

patrickkuo
Copy link
Contributor

a fix to enable optional input via sui json.

example:

call --package 0x2 --module sui_system --function request_add_delegation_mul_coin --args 0x5 [0x75a556740fb6e45219b1069ca08143ab086e9283, 0x907ae3c07cba82c26e363097e56f2aa0837547c2, 0xcf9b1967ee44e060c36a9b93d8075a0ccefce803] [200000000000000] 0x0879e3ec427b091fc22c1f765fdec22ea71a2150 --gas-budget 10000

We are using array to represent optional values, empty array [] indicate None.

@patrickkuo patrickkuo requested review from awelc and oxade January 4, 2023 12:50
@vercel
Copy link

vercel bot commented Jan 4, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Comments Updated
explorer-storybook ⬜️ Ignored (Inspect) Jan 11, 2023 at 4:58PM (UTC)
wallet-adapter ⬜️ Ignored (Inspect) Jan 11, 2023 at 4:58PM (UTC)

@patrickkuo patrickkuo self-assigned this Jan 4, 2023
Copy link
Collaborator

@sblackshear sblackshear left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link
Contributor

@awelc awelc left a comment

Choose a reason for hiding this comment

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

LGTM as well, but I think we should document this in https://github.com/MystenLabs/sui/blob/main/doc/src/reference/sui-json.md

@tnowacki
Copy link
Contributor

tnowacki commented Jan 5, 2023

Thanks! I think this solves #7142?

@patrickkuo
Copy link
Contributor Author

Thanks! I think this solves #7142?

Yes.

But I will revert the changes in Sui JSON and move the u64 to String magic to the CLI, the u64 String checks in Sui JSON is correct.

@patrickkuo
Copy link
Contributor Author

Moved the u64 -> String conversion to the CLI, also added more e2e tests.

Sui Json will continue to enforce u64 String.

@patrickkuo
Copy link
Contributor Author

LGTM as well, but I think we should document this in https://github.com/MystenLabs/sui/blob/main/doc/src/reference/sui-json.md

I am not changing the Sui JSON u64 string enforcement logic, so don't need to change the doc.

@awelc
Copy link
Contributor

awelc commented Jan 7, 2023

I am not changing the Sui JSON u64 string enforcement logic, so don't need to change the doc.

I was thinking of documenting how to encode Option in the Type coercion rules section unless you can think of a better place where developers would be able to find this info.

@patrickkuo
Copy link
Contributor Author

I am not changing the Sui JSON u64 string enforcement logic, so don't need to change the doc.

I was thinking of documenting how to encode Option in the Type coercion rules section unless you can think of a better place where developers would be able to find this info.

ah! of course! will add that

@github-actions github-actions bot added the Type: Documentation Improvements or additions to documentation label Jan 9, 2023
@patrickkuo
Copy link
Contributor Author

hi @awelc, does this looks ok to you?

@patrickkuo patrickkuo merged commit 0fb6465 into main Jan 11, 2023
@patrickkuo patrickkuo deleted the pat/suijson_optional_arg branch January 11, 2023 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants