stellar / js-stellar-base

The lowest-level stellar helper library. It consists of classes to read, write, hash, and sign Stellar xdr
https://stellar.github.io/js-stellar-base/
Apache License 2.0
108 stars 137 forks source link

Provide a better authorization abstraction, signing entries in-place #678

Closed Shaptic closed 1 year ago

Shaptic commented 1 year ago

Note for reviewers: I recommend reading the code directly rather than looking through the diff because it is quite messy after removing the existing helpers in 873d0fe. It also might help to read the new test cases.

What

This replaces the existing authorization helpers with a two methods:

export type SigningCallback = (
  preimage: xdr.HashIdPreimage
) => Promise<Buffer | Uint8Array | ArrayBuffer /* Buffer-like */>;

export function authorizeInvocation(
  signer: Keypair | SigningCallback,
  validUntil: number,
  invocation: xdr.SorobanAuthorizedInvocation,
  publicKey?: string,
  networkPassphrase?: string
): Promise<xdr.SorobanAuthorizationEntry>;

export function authorizeEntry(
  entry: xdr.SorobanAuthorizationEntry,
  signer: Keypair | SigningCallback,
  validUntilLedgerSeq: number,
  networkPassphrase?: string
): Promise<xdr.SorobanAuthorizationEntry>;

You can reference the jsdoc in the PR itself for more details.

Why

This redesign simplifies the way we can build entries, motivated by a quirk of Soroban: When simulating an invocation via Soroban RPC, the returned operation will indiscriminately have its auth[] array replaced with fresh (i.e. new nonces) and unsigned authorization entries. Originally, our intention was:

However, this is not ergonomic with preflight because:

We could do something to adjust the footprint on submission in soroban-client (e.g. more magic here, but this feels like a better, direct approach.

Known Limitations

This does mean that it's much more difficult to create an entry "from scratch" (i.e. from an invocation alone), but that seems like an edge case at this time. Perhaps we should still keep the old helpers (e.g. authorizeInvocation) rather than replacing them, but it's unclear whether or not those are useful to anybody? Maybe the introduction of #669 would help this along.

github-actions[bot] commented 1 year ago

Size Change: +11.2 kB (0%)

Total Size: 3.16 MB

Filename Size Change
dist/stellar-base.js 2.32 MB +8.12 kB (0%)
dist/stellar-base.min.js 843 kB +3.1 kB (0%)

compressed-size-action

paulbellamy commented 1 year ago

You may find this new bit of code in soroban-rpc preflight illuminating.

So, what we do in soroban-cli is:

  1. simulateTransaction to figure out the initial footprint, fees, & soroban auth entries
  2. sign any soroban authorization entries
  3. If we had any auth entries above, simulateTransaction again (with our signed auth entries included) to get the updated footprint & fees
  4. sign the txn envelope
  5. sendTransaction
Shaptic commented 1 year ago

@paulbellamy thank you for the CLI references! The difference in the SDK is two-fold:

(Does that sound right, @aristidesstaffieri?)

paulbellamy commented 1 year ago

No, the footprint from the second simulateTransaction should be correct in the next release of soroban-rpc, afaik.

Shaptic commented 1 year ago

@paulbellamy in the sense that it will leave auth entries untouched if present in the invokeHostFunction being simulated? :pray:

paulbellamy commented 1 year ago

If they are present on the txn it uses enforcing auth mode and doesn't alter the auth entries, yes.

Shaptic commented 1 year ago

That's promising! Then we can utilize the other abstractions.

aristidesstaffieri commented 1 year ago

@paulbellamy thank you for the CLI references! The difference in the SDK is two-fold:

  • signing is likely to be multi-machine (e.g. sending to another user or server), which isn't often true in the CLI case, so we can't just pass a signers: Keypair[] as that centralized everyone's private keys
  • this helper is distinct from the "assembly" step that would happen in the soroban-client portion (i.e. Server.prepareTransaction, where we already do misc. fee / auth / footprint adjustments). There, I entirely agree that we could do something like:
  1. simulateTransaction to get the footprint, fees, and auth
  2. kick that back to the user for them to figure out how they want to do auth, probably via authorizeEntry
  3. simulateTransaction again to get updated fees and leave auth untouched
  4. replace the footprint with the footprint from (1), because the second simulation would give us bs/irrelevant nonce footprints (one might call it "noncense" ayy lmao, see PR description for details)
  5. finally, sign & send

(Does that sound right, @aristidesstaffieri?)

Yeah that does sound right and is basically what current examples do.

aristidesstaffieri commented 1 year ago

@Shaptic I'm curious what the motivation is for providing authorizeInvocation? Thinking of this through the lends of an of an app workflow, I can't imagine a scenario that would be better suited for authorizeInvocation than authorizeEntry. When you need to sign an auth entry in practice, I would imagine it would almost always be pre-built for you.

Shaptic commented 1 year ago

@aristidesstaffieri that's a good point; I guess I'm thinking about advanced or optimized scenarios. For example, if any of the following isn't true, you'd want a authorizeInvocation to exist:

Maybe these are far fetched? But it still seems nice for flexibility at the stellar-base level to not necessarily explicitly rely on preflight to build the data structures you may need.

aristidesstaffieri commented 1 year ago

@aristidesstaffieri that's a good point; I guess I'm thinking about advanced or optimized scenarios. For example, if any of the following isn't true, you'd want a authorizeInvocation to exist:

  • you don't want to (or need to) do simulation for auth (or don't trust it)
  • you want to send around just the call tree to misc. parties (easier, smaller than the whole entry)
  • you received a JSONified call tree (e.g. the Freighter popup? or via buildInvocationTree) and want to build an entry from it

Maybe these are far fetched? But it still seems nice for flexibility at the stellar-base level to not necessarily explicitly rely on preflight to build the data structures you may need.

Not far fetched, those make a ton of sense. I just wanted to make sure I understood the use case.