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

soroban-cli: Add Auth-next signing support #749

Merged
merged 21 commits into from
Aug 15, 2023
Merged

soroban-cli: Add Auth-next signing support #749

merged 21 commits into from
Aug 15, 2023

Conversation

paulbellamy
Copy link
Contributor

@paulbellamy paulbellamy commented Jul 5, 2023

Fixes #683

Adds support for SorobanAuthorizationEntry (i.e. auth-next) multi-party signing in the soroban-cli.

Usage

# Here, the local user account `alice` is used to sign the txn,
# but `bob` is the one who's value will be incremented.
#
# The contract method signature for this is:
#     pub fn increment(env: Env, user: Address, value: u32) -> u32 { ... }
$ soroban contract invoke \
    --source alice \
    --id $auth_example_contract \
    -- \
    increment \
    --user bob \
    --value 5

Notes

To get this to work, I've had to change the rpc simulateTransaction endpoint. Now, if the txn includes any SorobanAuthorizationEntries, the simulation runs in enforcing auth mode. This means that the nonce the user sent will be used, generating the correct footprint and fees.

Todo

  • Try it out locally and check it actually works
  • Write a system-test test for this
  • update the RPC web docs for simulateTransaction update
  • create a follow-up issue to support passing keys directly on the CLI in the S... format

@paulbellamy
Copy link
Contributor Author

paulbellamy commented Jul 7, 2023

@dmkozh When I'm testing this, I keep getting TxInsufficientFee for any txn which involves auth-next signing...
For example, in this txn the only difference is the source-account (so one is Source credentials and the other is AddressNonce): https://gist.github.com/paulbellamy/5b30db0bb3046c145c921a39f4145e8b
Is there something else we need to do? We probably need to increase some fee for the size of the signatures right? IIRC there was some formula for that?

@dmkozh
Copy link
Contributor

dmkozh commented Jul 7, 2023

We probably need to increase some fee for the size of the signatures right?

Yes, as we don't support enforcing preflight yet, there are some gaps in terms of what we can meter in recording mode. Signature verification requires some additional instructions (though IIUC this didn't get to the stage where this is enforced). Also tx size will be increased a bit due to signatures. This should only need a small fee increase though and IIRC preflight already adds about 15% on top of the computed fee. So not sure what else could be missing.

@paulbellamy
Copy link
Contributor Author

paulbellamy commented Jul 10, 2023

It could be that the transaction is quite small, so 15% is not enough for the extra signature in my case.

What is the formula for increasing the size due to the signatures? Is it just adding those bytes onto the extended_meta_data_size_bytes? or the refundable_fee? or the fee? or somewhere else?

@dmkozh
Copy link
Contributor

dmkozh commented Jul 10, 2023

What is the formula for increasing the size due to the signatures?

The increased tx size needs to be passed to the fee computation library. It will be reflected in the final non-refundable resource fee. To be more specific, it will be included in tx size fee and historical fee. But really if you want to compute fees outside of the rust library, you should rather port the whole library (https://github.com/stellar/rs-soroban-env/blob/main/soroban-env-host/src/fees.rs)

Base automatically changed from cli-bump-restore-subcommands to soroban-xdr-next July 12, 2023 00:01
Base automatically changed from soroban-xdr-next to main July 12, 2023 13:01
cmd/soroban-cli/src/rpc/transaction.rs Show resolved Hide resolved
cmd/soroban-cli/src/rpc/transaction.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/rpc/transaction.rs Outdated Show resolved Hide resolved
@paulbellamy paulbellamy force-pushed the cli-auth-next branch 3 times, most recently from abda5b8 to cac6610 Compare August 7, 2023 16:18
@paulbellamy paulbellamy marked this pull request as ready for review August 11, 2023 16:53
@paulbellamy paulbellamy enabled auto-merge (squash) August 11, 2023 16:57
@paulbellamy paulbellamy merged commit 66778f7 into main Aug 15, 2023
20 of 21 checks passed
@paulbellamy paulbellamy deleted the cli-auth-next branch August 15, 2023 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

soroban-cli: update to support new auth and single host function
6 participants