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 no-build option for fee::Args #1267

Merged
merged 10 commits into from
May 9, 2024
Merged

Conversation

willemneal
Copy link
Member

@willemneal willemneal commented Mar 20, 2024

Fixes #1265.
Requires: #1173

Commands that build transactions now include the following args to output and exit early:

  • --build-only prints the base64 xdr before simulation
  • --sim-only prints the base64 xdr after simulation

For example,

soroban contract invoke --build-only .. -- hello --world world

prints:
AAAAAMwWyQMtAWO6nW4s+cKMRunx+kiaKfjJwIRNZEEclDvRAAAAZAAIgAEAAAANAAAAAAAAAAAAAAABAAAAAAAAABgAAAAAAAAAAQ3DO777M8sdGtPbMau+cuxAles4UILfUkZXOWTsqKvOAAAABWhlbGxvAAAAAAAAAQAAAA8AAAAFd29ybGQAAAAAAAAAAAAAAA==

soroban contract invoke --sim-only .. -- hello --world world

AAAAAMwWyQMtAWO6nW4s%2BcKMRunx%2BkiaKfjJwIRNZEEclDvRAAD4WAAIgAEAAAANAAAAAAAAAAAAAAABAAAAAAAAABgAAAAAAAAAAQ3DO777M8sdGtPbMau%2BcuxAles4UILfUkZXOWTsqKvOAAAABWhlbGxvAAAAAAAAAQAAAA8AAAAFd29ybGQAAAAAAAAAAAAAAQAAAAAAAAACAAAABgAAAAENwzu%2B%2BzPLHRrT2zGrvnLsQJXrOFCC31JGVzlk7KirzgAAABQAAAABAAAAB3mMSQSdZWZNBqeYQchZLRMLUhSFaxsEJAOkX%2Bel%2B%2BMzAAAAAABcQYMAABsoAAAAAAAAAAAAANeQ

Transaction result

The NetworkRunable trait a result type of TxnResult, either a raw xdr string or a T. This allows returning early.

update is-view

Ensure that is-view doesn't fetch account info as a source account doesn't need to exist for a simulation.

@willemneal willemneal force-pushed the feat-add-no_build-to-fee-Args branch 2 times, most recently from 2c5050f to dc2e10d Compare March 20, 2024 16:42
@willemneal willemneal force-pushed the feat-add-no_build-to-fee-Args branch from 3b9ad3b to 398a018 Compare March 20, 2024 17:19
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.

That was fast! This is great 👏🏻.

Couple questions inline.

The --sim-only might be unnecessary, given that folks could do soroban contract deploy --build-only | soroban tx simulate in the future, and that leans on command composability and keeps the options simpler. Thoughts from others on this? cc @fnando @chadoh

cmd/soroban-cli/src/fee.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/fee.rs Outdated Show resolved Hide resolved
@leighmcculloch
Copy link
Member

Will take another look once #1173 is merged, but this looked good when I last reviewed other than the couple minor things which are addressed so I think we'll be able to fast merge this once the other is on its way.

@willemneal willemneal force-pushed the feat-add-no_build-to-fee-Args branch from f53e8b1 to 816e470 Compare April 23, 2024 19:49
@willemneal willemneal force-pushed the feat-add-no_build-to-fee-Args branch 2 times, most recently from 71ecb56 to 4d20c27 Compare May 3, 2024 14:23
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.

Few questions inline. Looks like this isn't far off.

cmd/soroban-cli/src/commands/contract/deploy/wasm.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/commands/contract/deploy/asset.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/fee.rs Outdated Show resolved Hide resolved
@willemneal willemneal enabled auto-merge (squash) May 7, 2024 22:17
@willemneal willemneal force-pushed the feat-add-no_build-to-fee-Args branch from 83157b3 to 9d7bee9 Compare May 7, 2024 22:19
@willemneal willemneal force-pushed the feat-add-no_build-to-fee-Args branch from 9d7bee9 to 58daf6c Compare May 8, 2024 20:27
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 pushed a couple changes to the PR in the last 3 commits, and I left comments inline to outline why. Please take a look and let me know if you agree with them.

@leighmcculloch
Copy link
Member

@willemneal If you're good with the changes I pushed, can you resolve the unresolved conversations which are just the notes I left explaining why each change, and once they're all resolved that should trigger GitHub to auto-merge the PR.

If possible don't force push the branch in case we need to make more changes.

@leighmcculloch
Copy link
Member

I pushed a bug fix in 457a54b for how sim only works with install. I left a comment in the code about it for more information.

@willemneal willemneal merged commit 199a88a into main May 9, 2024
18 of 19 checks passed
@willemneal willemneal deleted the feat-add-no_build-to-fee-Args branch May 9, 2024 12:31
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.

Separate tx build from simulate/sign/send
2 participants