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 tx set #1760

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat: add tx set #1760

wants to merge 4 commits into from

Conversation

willemneal
Copy link
Member

@willemneal willemneal commented Nov 26, 2024

Currently allows updating fields on Transactions V1 only.

Close #1643

Currently allows updating fields on Transactions V1 only.
@willemneal willemneal self-assigned this Nov 26, 2024
@willemneal willemneal marked this pull request as ready for review November 26, 2024 21:00
@sagpatil sagpatil linked an issue Nov 26, 2024 that may be closed by this pull request
Comment on lines +22 to +29
/// 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),
Copy link
Member

Choose a reason for hiding this comment

The 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.:

  • New
  • Set
  • Simulate
  • Sign
  • Send

Comment on lines +12 to +14
XdrStdin(#[from] super::xdr::Error),
#[error(transparent)]
Xdr(#[from] xdr::Error),
Copy link
Member

Choose a reason for hiding this comment

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

Why are there two distinct xdr error types in use here? There should only ever be one in the cli in use.

Comment on lines +1767 to +1790
## `stellar tx set`

Set various options for a transaction

**Usage:** `stellar tx set [OPTIONS]`

###### **Options:**

* `--sequence-number <SEQUENCE_NUMBER>` — Set the transactions sequence number
* `--fee <FEE>` — Set the transactions fee
* `--memo-text <MEMO_TEXT>` — Set the transactions memo text
* `--memo-id <MEMO_ID>` — Set the transactions memo id
* `--memo-hash <MEMO_HASH>` — Set the transactions memo hash
* `--memo-return <MEMO_RETURN>` — Set the transactions memo return
* `--source-account <SOURCE_ACCOUNT>` — Change the source account for the transaction
* `--max-time-bound <MAX_TIME_BOUND>` — Set the transactions max time bound
* `--min-time-bound <MIN_TIME_BOUND>` — Set the transactions min time bound
* `--min-ledger <MIN_LEDGER>` — Set the minimum ledger that the transaction is valid
* `--max-ledger <MAX_LEDGER>` — Set the max ledger that the transaction is valid. 0 or not present means to max
* `--min-seq-num <MIN_SEQ_NUM>` — set mimimum sequence number
* `--min-seq-age <MIN_SEQ_AGE>`
* `--min-seq-ledger-gap <MIN_SEQ_LEDGER_GAP>` — min sequeence ledger gap
* `--extra-signers <EXTRA_SIGNERS>` — Extra signers
* `--no-preconditions` — Set precondition to None
Copy link
Member

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:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Needs Review
Development

Successfully merging this pull request may close these issues.

How to set fields on Transactions
2 participants