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

[fastx client & benchmark] Updated client wrapper to use objects. Updated readme commands #91

Merged
merged 20 commits into from
Dec 30, 2021

Conversation

oxade
Copy link
Contributor

@oxade oxade commented Dec 24, 2021

What
Balances are still used in parts of the code.
Why
Part of broader move to object model
This has to be removed as a precursor to benchmarking work since scripts and benchmarking scripts are using old logic.
Limitations
Client wrapper used to accept explicit balances, but now randomly generates ObjectIDs for use. Will enhance in future to use custom IDs or IDs from actual objects/resources
Transfer issue found: #105

@oxade oxade self-assigned this Dec 24, 2021
fastpay/src/config.rs Outdated Show resolved Hide resolved
fastpay/src/server.rs Outdated Show resolved Hide resolved
fastpay/src/server.rs Outdated Show resolved Hide resolved
fastpay/src/client.rs Show resolved Hide resolved
@oxade oxade removed their assignment Dec 26, 2021
@sblackshear
Copy link
Collaborator

sblackshear commented Dec 27, 2021

TOML fixes look good--thanks for that!

My remaining question is still #91 (comment) think we should decide on that and make sure this PR is moving us toward our goal.

How do you see CreateAccounts being used going forward? I could imagine, e.g.,

create_accounts 10 (creates 10 gas currency objects with some default value)
create_accounts 10 --value 150 (creates 10 gas currency objects with value 50)
create_accounts 10 20 30 (creates 3 gas currency objects with value 10, 20, 30)

fastpay/Cargo.toml Outdated Show resolved Hide resolved
fastpay/src/client.rs Outdated Show resolved Hide resolved
fastpay/src/client.rs Outdated Show resolved Hide resolved
fastpay/src/config.rs Outdated Show resolved Hide resolved
fastpay/src/config.rs Show resolved Hide resolved
fastpay/src/server.rs Outdated Show resolved Hide resolved
fastpay/src/config.rs Outdated Show resolved Hide resolved
fastpay/src/client.rs Outdated Show resolved Hide resolved
fastpay/src/client.rs Outdated Show resolved Hide resolved
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.

It seems like the commands do a mix of printing output and writing it to a file. We should probably pick one output mechanism and stick to it?

README.md Outdated Show resolved Hide resolved
fastpay/src/client.rs Outdated Show resolved Hide resolved
fastpay/src/client.rs Outdated Show resolved Hide resolved
fastpay/src/client.rs Outdated Show resolved Hide resolved
Co-authored-by: Sam Blackshear <sam@mystenlabs.com>
@oxade
Copy link
Contributor Author

oxade commented Dec 29, 2021

It seems like the commands do a mix of printing output and writing it to a file. We should probably pick one output mechanism and stick to it?

@sblackshear yes. I largely stuck with them to maintain parity with the commands from the ReadMe examples which are doing some basic bash parsing on the outputs.
But it makes sense to overhaul this and only print for commands that execute get queries.

@oxade oxade changed the title Updated client wrapper to use objects. Updated readme commands [fastx client] Updated client wrapper to use objects. Updated readme commands Dec 29, 2021
fastpay/src/client.rs Outdated Show resolved Hide resolved
fastpay/src/client.rs Outdated Show resolved Hide resolved
fastpay/src/client.rs Outdated Show resolved Hide resolved
@oxade oxade changed the title [fastx client] Updated client wrapper to use objects. Updated readme commands [fastx client & benchmark] Updated client wrapper to use objects. Updated readme commands Dec 29, 2021
@sblackshear
Copy link
Collaborator

After studying at the client code a bit more, I don't think swapping out the "output to a file" logic for printing is moving in the right direction. The design of the existing client (as far as I can tell) is that you do queries to sync the latest state to a file, then use that file for subsequent operations (e.g., preparing txes to send). If we take away the file, we'll need to add a new mechanism for doing this later.

I like the changes this PR is making to CreateAccounts, but I think it should save the results to a file in the same style as the existing command.

@oxade
Copy link
Contributor Author

oxade commented Dec 29, 2021

After studying at the client code a bit more, I don't think swapping out the "output to a file" logic for printing is moving in the right direction. The design of the existing client (as far as I can tell) is that you do queries to sync the latest state to a file, then use that file for subsequent operations (e.g., preparing txes to send). If we take away the file, we'll need to add a new mechanism for doing this later.

I like the changes this PR is making to CreateAccounts, but I think it should save the results to a file in the same style as the existing command.

Ah you're talking about QueryObjects?
Yes will address that to sync to file

@sblackshear
Copy link
Collaborator

  • Yes, my bad, I meant QueryObjects instead of CreateAccounts (specifically, the deletion at 449)
  • I'm agnostic on printing in addition to saving to a file. If the goal of this PR is to update the existing wrapper API's to use objects, maybe stick to that + file a task for adding printing to all commands (not just the ones being updated by this command)? It's worth noting that the more Rust-idiomatic way to do this sort of printing would be to have each command return a structured object that implements Display + allow the client to toggle printing on and/off with (e.g.) a -q flag, so perhaps worth hashing that out in a separate PR.

@oxade
Copy link
Contributor Author

oxade commented Dec 30, 2021

  • Yes, my bad, I meant QueryObjects instead of CreateAccounts (specifically, the deletion at 449)

Agreed. Will address this.

  • I'm agnostic on printing in addition to saving to a file. If the goal of this PR is to update the existing wrapper API's to use objects, maybe stick to that + file a task for adding printing to all commands (not just the ones being updated by this command)? It's worth noting that the more Rust-idiomatic way to do this sort of printing would be to have each command return a structured object that implements Display + allow the client to toggle printing on and/off with (e.g.) a -q flag, so perhaps worth hashing that out in a separate PR.

Will rather do it properly in the Rust-style. Will open a task for this: #104

@oxade oxade merged commit 9514c2f into main Dec 30, 2021
@huitseeker huitseeker deleted the remove-payment-vestiges branch February 2, 2022 13:06
mwtian pushed a commit that referenced this pull request Sep 12, 2022
…ons on primary (#91)

[feature]: this commit introduces a new component that orchestrates the collections removal from our system.
mwtian pushed a commit to mwtian/sui that referenced this pull request Sep 29, 2022
…ons on primary (MystenLabs#91)

[feature]: this commit introduces a new component that orchestrates the collections removal from our system.
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.

3 participants