Skip to content

feat: txn command to simulate - #1283

Merged
willemneal merged 44 commits into
mainfrom
feat/txn-command
Jun 21, 2024
Merged

feat: txn command to simulate#1283
willemneal merged 44 commits into
mainfrom
feat/txn-command

Conversation

@willemneal

@willemneal willemneal commented Apr 15, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Part of #1265
Adds

  • txn simulate - simulate and assemble transaction

Future PR:

  • signer::Stellar trait and a default InMemory implementation needed for txn sign
  • txn sign - sign a given transaction
  • txn inspect
  • txn send - sends a signed transaction to the PRC (not useful without sign)

Left

  • Tests

@willemneal
willemneal force-pushed the feat/txn-command branch 6 times, most recently from 2d6ae5b to 0b4cb92 Compare April 16, 2024 17:00
@willemneal
willemneal marked this pull request as ready for review April 16, 2024 20:03
@elizabethengelman elizabethengelman mentioned this pull request Apr 25, 2024
Merged
5 tasks
This was referenced Apr 30, 2024
Closed
Closed
@willemneal
willemneal force-pushed the feat/txn-command branch 3 times, most recently from 578db4f to 7609f30 Compare May 7, 2024 18:43
@janewang janewang added this to the v21 milestone May 7, 2024
leighmcculloch
leighmcculloch reviewed May 10, 2024
View reviewed changes
Comment thread cmd/soroban-cli/src/commands/tx/mod.rs Outdated
Comment thread cmd/soroban-cli/src/commands/tx/mod.rs Outdated
Comment thread cmd/soroban-cli/src/commands/tx/send.rs Outdated
Comment thread cmd/soroban-cli/src/commands/tx/sign.rs Outdated
Comment thread cmd/soroban-cli/src/commands/tx/sign.rs Outdated
Comment thread cmd/soroban-cli/src/commands/tx/xdr.rs Outdated
leighmcculloch
leighmcculloch reviewed May 10, 2024
View reviewed changes
Comment thread cmd/soroban-cli/src/signer.rs Outdated
leighmcculloch
leighmcculloch reviewed May 10, 2024
View reviewed changes

@leighmcculloch leighmcculloch left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I pushed some commits getting main merged in.

This PR looks like it's still a little ways away from being ready. Left some inline comments.

Would be a good idea to add tests for these commands too, from the outside-in.

@leighmcculloch

leighmcculloch commented May 10, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Given the sensitivity of the sign command, I'd like to shift that work into a separate PR, so that we can get the other commands merged sooner, then follow up and take our time with the sign command. There should be litle to no need for the commands to be added together, so I think that'll be fine, but please lmk @willemneal if you think that'd be a problem. cc @janewang

leighmcculloch
leighmcculloch reviewed May 10, 2024
View reviewed changes
Comment thread cmd/soroban-cli/src/commands/tx/xdr.rs Outdated
@willemneal

willemneal commented May 28, 2024

Copy link
Copy Markdown
Contributor Author

@leighmcculloch I've removed send because it has hard to test without a proper sign command. So there is another PR to add all the remaining besides inspect.

@leighmcculloch leighmcculloch changed the title feat: txn commands to sign, simulate, and send txns feat: txn command to simulate May 28, 2024
@willemneal
willemneal enabled auto-merge (squash) May 31, 2024 15:40
@willemneal willemneal mentioned this pull request May 31, 2024
Closed
@willemneal
willemneal requested a review from leighmcculloch June 3, 2024 16:57
chadoh
chadoh reviewed Jun 21, 2024
View reviewed changes

@chadoh chadoh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice clean code! I really like the tests. I have some questions, before I approve.

Comment thread cmd/crates/soroban-test/tests/it/integration/tx.rs Outdated
Comment thread cmd/crates/soroban-test/tests/it/integration/tx.rs Outdated
Comment thread cmd/crates/soroban-test/tests/it/integration/tx.rs Outdated
Comment thread cmd/crates/soroban-test/tests/it/integration/tx.rs
Comment thread cmd/soroban-cli/src/commands/tx/mod.rs
Comment thread cmd/soroban-cli/src/commands/tx/simulate.rs Outdated
Comment thread cmd/crates/soroban-test/tests/it/integration/tx.rs Outdated
@willemneal
willemneal requested a review from chadoh June 21, 2024 19:22
chadoh
chadoh approved these changes Jun 21, 2024
View reviewed changes

@chadoh chadoh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks great, thanks!

@willemneal
willemneal merged commit 98900d5 into main Jun 21, 2024
@willemneal
willemneal deleted the feat/txn-command branch June 21, 2024 22:40
@willemneal willemneal mentioned this pull request Jun 25, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@leighmcculloch leighmcculloch Awaiting requested review from leighmcculloch
1 more reviewer
@chadoh chadoh chadoh approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

DevX
Status: Done

Milestone

v21

Development

Successfully merging this pull request may close these issues.

4 participants

@willemneal @leighmcculloch @chadoh @janewang