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::builder module and initial classic commands #1551

Merged
merged 65 commits into from
Oct 2, 2024

Conversation

willemneal
Copy link
Member

@willemneal willemneal commented Aug 19, 2024

What

Add a transaction builder and operation builder trait. Also adds the first two stellar classic commands under the subcommand tx new:

  • Create account
  • Payment
  • Set options
  • Bump sequence
  • Account Merge
  • Manage data
  • InvokeFunction
  • Extend footprint ttl
  • Restore footprint
  • Allow trust
  • Change trust
  • Set trustline flags
  • Doc review

Todo:

  • Tests

Why

Currently all transactions are built by hand and require a lot of knowledge around the internal types. Furthermore, there is a lot of redundant code around creating, submitting transaction.

These examples should make it easy to have a quick design discussion before adding the rest of the operations.

Known limitations

[TODO or N/A]

Currently all transactions are built by hand and require a lot of knowledge around the internal types. Furthermore, there is a lot of redundant code around creating, submitting transaction.

This PR adds the two new operation builders (create account and payment) and their corresponding commands.

This example should make it easy to have a quick design discussion before adding the rest of the operations.
@willemneal willemneal changed the title feat: add tx::builder and initial classic cmds createAccount/payment feat: add tx::builder module and initial classic commands create account & payment Aug 19, 2024
Users can use `builders::MuxedAccount` to create the account if needed, but most cases won't need it
@willemneal willemneal linked an issue Aug 20, 2024 that may be closed by this pull request
11 tasks
@willemneal willemneal changed the title feat: add tx::builder module and initial classic commands create account & payment feat: add tx::builder module and initial classic commands Aug 22, 2024
Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

I still need to do a full pass but a cursory look and this is very elegant. I appreciate that we're consolidating the building of some existing txs in the CLI along the way. Some questions and thoughts inline, specifically to avoid recreating another Stellar SDK-like layer, but that might be necessary at this point.

I've got this in my review queue to do a full review, just ran out of time to go deep today.

cmd/crates/soroban-test/Cargo.toml Outdated Show resolved Hide resolved
cmd/soroban-cli/src/tx/builder/bytesm.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/tx/builder/muxed_account.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/tx/builder/transaction.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/commands/tx/args.rs Outdated Show resolved Hide resolved
@Ifropc Ifropc self-requested a review September 25, 2024 18:45
@willemneal
Copy link
Member Author

@leighmcculloch Updated to reflect feedback. Still need to update tests.

cmd/soroban-cli/src/commands/tx/new/change_trust.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/commands/tx/new/mod.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/commands/tx/new/mod.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/commands/tx/new/mod.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/tx/builder/transaction.rs Outdated Show resolved Hide resolved
Also use `From<&Cmd> for xdr::OperationBody` instead of creating a new type. Then use this to simplify each command by moving the `tx.handle_and_print` to the parent subcommand so that each command only needs to implement the trait.
@willemneal
Copy link
Member Author

@leighmcculloch, I've removed all of the builder types that I could. One that still needs to be added to the xdr crate is the FromStr for Asset, which I will open another PR for soon. Also I removed the unneeded Operation trait and just use From for Operation Body, all of which simplified the code a good deal.

Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

👏🏻

cmd/soroban-cli/src/commands/tx/args.rs Show resolved Hide resolved
cmd/soroban-cli/src/commands/tx/new/set_trustline_flags.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/config/address.rs Show resolved Hide resolved
@leighmcculloch
Copy link
Member

@janewang @stellar/devx There's a large amount of functionality being introduced with this PR. We should plan to do some dog fooding of it for ergonomics in the next couple weeks, maybe get some others to dog food it too.

@leighmcculloch leighmcculloch enabled auto-merge (squash) October 2, 2024 05:41
@leighmcculloch leighmcculloch merged commit 1f20dcd into stellar:main Oct 2, 2024
26 checks passed
@willemneal willemneal deleted the feat/tx_builder branch October 2, 2024 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Enable transaction creation for critical classic commands
3 participants