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 submitting txs to the cli #159

Closed
wants to merge 8 commits into from

Conversation

davekaj
Copy link
Contributor

@davekaj davekaj commented Aug 13, 2024

This PR started by addresses #145 but I just decided to make it a smaller PR that just adds the ability to send a TX through the CLI, so I could get familiar with the CLI code before I start #145. So it will not close #145.

What this PR does:

  • Adds in a subcommand to contract that allows you to send a tx to the contract. Example: cargo run -- contract tx --msg "{\"query_request\": {\"emphemeral_pubkey\": \"$EPHEMERAL_PUBKEY\"}}" --contract $CONTRACT
  • When a tx is sent, it also waits 5 seconds to see if the tx is successful, and if not it will return the error (this helps a lot with debugging, as the raw wasmd or neutrond commands do not return the status)
  • Also adds more detailed error messages to wasmd_client.query_tx()
  • Adds the option to pass a base token amount to wasmd_client.tx_execute()
  • Adds to the cli README all the necessary commands to help users get started using the cli and interacting with the enclave for the transfers app

@davekaj davekaj requested a review from dangush August 14, 2024 13:46
Comment on lines +46 to +51
let hash = tx_output.txhash.to_string();
info!("🚀 Successfully sent tx with hash {}", hash);
info!("Waiting 5 seconds for tx to be included in block.....");

// TODO - a more robust timeout mechanism with polling blocks
sleep(Duration::from_secs(5)).await;
Copy link
Contributor

Choose a reason for hiding this comment

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

A function with this polling mechanism exists in handler/utils/helpers.rs called block_tx_commit

@dangush
Copy link
Contributor

dangush commented Aug 14, 2024

Changes look good,

but what's the difference between this and wasmd tx?

@davekaj
Copy link
Contributor Author

davekaj commented Aug 15, 2024

Changes look good,

but what's the difference between this and wasmd tx?

Yes there isn't much difference right now. The main benefits now:

  • encapsulating everything in the cli, which I believe will be easier for quartz developers to wrap their head around.
  • better error handling - right now whenever I test with wasmd tx I often then have to do a wasmd query to see if the tx was successful
  • we won't have to write docs with wasmd neutrond or our-testnet cmds, we can just run our cli and if we set the config to wasmd or neutrond it should just work. and the cli should be app agnostic, so other blockchains could leverage it, and they likely won't use wasmd or neutrond

Longer term, we can build in even better error handling, support different networks through the cli (neutron, wasmd, our testnet), and I imagine for our own testnet we would want our CLI to have this functionality, since we wouldn't be using wasmd or neutrond. We could also do better handling of the msg args, instead of having to pass messy json strings with slashes in them.

All in all it's a minor addition. Let me know if you think it is valuable enough to include. If so, I will merge main into here, and use block_commit_tx() as suggested.

@davekaj davekaj changed the title [DRAFT] feat: add submitting txs and reading querying tx hashes to the cli feat: add submitting txs and reading querying tx hashes to the cli Aug 16, 2024
@davekaj davekaj changed the title feat: add submitting txs and reading querying tx hashes to the cli feat: add submitting txs to the cli Aug 16, 2024
@davekaj davekaj marked this pull request as ready for review August 16, 2024 12:47
@dangush
Copy link
Contributor

dangush commented Aug 16, 2024

Support for multiple networks makes sense! What's the difference between the neutron and wasmd tools for submitting a tx right now?

What happens if you try to submit a tx using wasmd but pointed at a Neutron node url?

Asking because I'm wondering if we should just add this network switching feature now with this pr

@davekaj
Copy link
Contributor Author

davekaj commented Aug 16, 2024

Theres no support for neutrond right now.
i actually dont know what would happen if you sent a wasmd tx to a neutrond node.... possible it might still work, but still not good practice

seems like we would update wasmd_client.rs to be something like blockchain_client.rs and export a wasmd client and a neutrond client, and any others, that are all very similar

i agree it would be useful to add in this PR, let me look into it and get back to you here

@davekaj
Copy link
Contributor Author

davekaj commented Aug 19, 2024

our discussion here would close out #75

We could just merge this as is, and work on the neutron deployment in a different PR, which is the simplest way forward

@davekaj davekaj mentioned this pull request Aug 20, 2024
@ebuchman
Copy link
Member

Nice idea but too porcelain for now. Also it's only good for sending txs but maybe doesnt cover all flags options. And doesn't cover querying (eg. balances) or managing keys which would still require the underlying wasmd binary anyways.

Theres a new readme instructions in here that we might want to rescue but needs to be updated and use the quartz bin instead of cargo run

@ebuchman ebuchman closed this Aug 27, 2024
@davekaj davekaj deleted the dave/145-query-enclave-directly branch December 12, 2024 19:39
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.

Quartz v0.1 MVP version of querying the enclave directly for user encrypted data
3 participants