-
Notifications
You must be signed in to change notification settings - Fork 75
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 tx set
#1760
Open
willemneal
wants to merge
4
commits into
main
Choose a base branch
from
feat/tx_set
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+499
−68
Open
feat: add tx set
#1760
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
use soroban_cli::xdr::{Limits, ReadXdr, TransactionEnvelope}; | ||
use soroban_test::{AssertExt, TestEnv}; | ||
|
||
use crate::integration::util::HELLO_WORLD; | ||
|
||
#[tokio::test] | ||
async fn build_simulate_sign_send() { | ||
let sandbox = &TestEnv::new(); | ||
let tx_base64 = sandbox | ||
.new_assert_cmd("contract") | ||
.arg("install") | ||
.args([ | ||
"--wasm", | ||
HELLO_WORLD.path().as_os_str().to_str().unwrap(), | ||
"--build-only", | ||
]) | ||
.assert() | ||
.success() | ||
.stdout_as_str(); | ||
let tx_env = TransactionEnvelope::from_xdr_base64(&tx_base64, Limits::none()).unwrap(); | ||
// set transaction options set fee | ||
let new_tx = sandbox | ||
.new_assert_cmd("tx") | ||
.arg("set") | ||
.arg("--fee") | ||
.arg("10000") | ||
.write_stdin(tx_base64.as_bytes()) | ||
.assert() | ||
.success() | ||
.stdout_as_str(); | ||
let tx_env_two = TransactionEnvelope::from_xdr_base64(&new_tx, Limits::none()).unwrap(); | ||
let tx = soroban_cli::commands::tx::xdr::unwrap_envelope_v1(tx_env_two).unwrap(); | ||
assert_eq!(tx.fee, 10000); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,5 +6,6 @@ mod init; | |
#[cfg(feature = "it")] | ||
mod integration; | ||
mod plugin; | ||
mod tx; | ||
mod util; | ||
mod version; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
use soroban_cli::xdr::{Limits, ReadXdr, TransactionEnvelope}; | ||
use soroban_test::{AssertExt, TestEnv}; | ||
|
||
const SOURCE: &str = "GBZXN7PIRZGNMHGA7MUUUF4GWPY5AYPV6LY4UV2GL6VJGIQRXFDNMADI"; | ||
|
||
#[tokio::test] | ||
async fn build_simulate_sign_send() { | ||
let sandbox = &TestEnv::new(); | ||
let tx_base64 = sandbox | ||
.new_assert_cmd("tx") | ||
.args(["new", "payment", "--destination", SOURCE, "--amount", "222"]) | ||
.assert() | ||
.success() | ||
.stdout_as_str(); | ||
let tx_env = TransactionEnvelope::from_xdr_base64(&tx_base64, Limits::none()).unwrap(); | ||
let tx = soroban_cli::commands::tx::xdr::unwrap_envelope_v1(tx_env).unwrap(); | ||
assert_eq!(tx.fee, 100); | ||
// set transaction options set fee | ||
let new_tx = sandbox | ||
.new_assert_cmd("tx") | ||
.arg("set") | ||
.arg("--fee") | ||
.arg("10000") | ||
.write_stdin(tx_base64.as_bytes()) | ||
.assert() | ||
.success() | ||
.stdout_as_str(); | ||
let tx_env_two = TransactionEnvelope::from_xdr_base64(&new_tx, Limits::none()).unwrap(); | ||
let tx = soroban_cli::commands::tx::xdr::unwrap_envelope_v1(tx_env_two).unwrap(); | ||
assert_eq!(tx.fee, 10000); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,11 @@ | ||
use super::global; | ||
|
||
pub mod args; | ||
|
||
pub mod hash; | ||
pub mod new; | ||
pub mod send; | ||
pub mod set; | ||
pub mod sign; | ||
pub mod simulate; | ||
pub mod xdr; | ||
|
@@ -12,17 +14,19 @@ pub use args::Args; | |
|
||
#[derive(Debug, clap::Subcommand)] | ||
pub enum Cmd { | ||
/// Simulate a transaction envelope from stdin | ||
Simulate(simulate::Cmd), | ||
/// Calculate the hash of a transaction envelope from stdin | ||
Hash(hash::Cmd), | ||
/// Sign a transaction envelope appending the signature to the envelope | ||
Sign(sign::Cmd), | ||
/// Send a transaction envelope to the network | ||
Send(send::Cmd), | ||
/// Create a new transaction | ||
#[command(subcommand)] | ||
New(new::Cmd), | ||
/// Send a transaction envelope to the network | ||
Send(send::Cmd), | ||
/// Set various options for a transaction | ||
Set(set::Cmd), | ||
/// Simulate a transaction envelope from stdin | ||
Simulate(simulate::Cmd), | ||
/// Sign a transaction envelope appending the signature to the envelope | ||
Sign(sign::Cmd), | ||
Comment on lines
+22
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we organise these commands by order of likely to use next? i.e.:
|
||
} | ||
|
||
#[derive(thiserror::Error, Debug)] | ||
|
@@ -37,6 +41,8 @@ pub enum Error { | |
Sign(#[from] sign::Error), | ||
#[error(transparent)] | ||
Send(#[from] send::Error), | ||
#[error(transparent)] | ||
Set(#[from] set::Error), | ||
} | ||
|
||
impl Cmd { | ||
|
@@ -47,6 +53,7 @@ impl Cmd { | |
Cmd::New(cmd) => cmd.run(global_args).await?, | ||
Cmd::Sign(cmd) => cmd.run(global_args).await?, | ||
Cmd::Send(cmd) => cmd.run(global_args).await?, | ||
Cmd::Set(cmd) => cmd.run(global_args)?, | ||
}; | ||
Ok(()) | ||
} | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Playing around with this interface I think some design discussion is required for this sub-command.
When we previously discussed this (#1643 (comment)) I had suggested we could have one sub-command per field to set, but I think this arrangement is also reasonable, but looks like there's missing functionality as it is, so we need that design to flesh out all the use cases.
For example, it looks like we can clear all the preconditions with 'no-preconditions', but it's not obvious to a user which fields are being cleared because the options are bundled in a single command the relationship between all the options appear equal / related, when they are not.
Another example, there's no way to remove a memo, or some precondition fields without clearing all preconditions.
I've started a design discussion on the issue: