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.

Sign up for GitHub

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

Jump to bottom

fix: make empty array the default function argument #84

Merged
merged 6 commits into from
Mar 8, 2022
Merged

fix: make empty array the default function argument #84

merged 6 commits into from
Mar 8, 2022

Conversation

itegulov
Copy link
Contributor

@itegulov itegulov commented Mar 3, 2022

Fixes #36

@itegulov itegulov requested a review from ChaoticTempest March 3, 2022 01:43
@ChaoticTempest
Copy link
Member

@itegulov I forgot, there's one thing we should correct before pulling this in. The ref_finance example has some calls into function with optionals, which will error out with this PR. Can you fix that when you get the chance. We should be able to see which one fails with #83 being pulled in

@itegulov
Copy link
Contributor Author

itegulov commented Mar 3, 2022

@ChaoticTempest indeed, should be fixed now. Do you want me to convert these examples to be tests and add them to CI in a separate PR?

@ChaoticTempest
Copy link
Member

ChaoticTempest commented Mar 4, 2022 •
edited
Loading

it be cool if we had a test that tests for optional args specifically, so don't need to pull all of the example in to tests. Either in this PR or another one is fine. Whichever works better for you

@itegulov
Copy link
Contributor Author

itegulov commented Mar 8, 2022

@ChaoticTempest added. PTAL when you can.

ChaoticTempest
ChaoticTempest reviewed Mar 8, 2022
View reviewed changes
workspaces/tests/optional_args.rs
.deposit(1)
.transact()
.await;
assert!(format!("{:?}", res.unwrap_err()).contains("Failed to deserialize input from JSON"));
Copy link
Member

Choose a reason for hiding this comment

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

yeah this error message is pretty unintuitive, so best for us to change this on the SDK side in the future

@itegulov itegulov merged commit 7f7e133 into main Mar 8, 2022
@itegulov itegulov deleted the daniyar/default-arguments branch March 8, 2022 23:09
@frol frol mentioned this pull request Oct 4, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChaoticTempest ChaoticTempest ChaoticTempest approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Investigate inner JSON RPC serialization
2 participants
@itegulov @ChaoticTempest