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!: enable cross-contract auth #1044

Merged
merged 1 commit into from
Oct 3, 2024

Conversation

chadoh
Copy link
Contributor

@chadoh chadoh commented Sep 10, 2024

Fixes: #1030

Soroban allows contract authors to call require_auth() not only on a
G-address (for users), but also on a C-address (for contracts). This
will result in a cross-contract call to the __check_auth function in
the signing contract. This enables all sorts of powerful and novel
use-cases. See https://developers.stellar.org/docs/build/guides/conventions/check-auth-tutorials#customaccountinterface-implementation

Just as if one account submits a transaction (meaning that account signs
the transaction envelope) but the contract calls require_auth on a
different one, the app author will need to use
needsNonInvokerSigningBy and signAuthEntries, so app authors benefit
from these functions when require_auth is called on a contract.

This fixes various assumptions that these functions made to also work
with this sort of cross-contract auth.

  • needsNonInvokerSigningBy now returns contract addresses
  • sign ignores contracts in the needsNonInvokerSigningBy list, since
    the actual authorization will happen via cross-contract call
  • signAuthEntry now takes an address instead of a publicKey, since
    this address may be a user's public key (a G-address) or a contract
    address. Furthermore, it allows setting a custom authorizeEntry, so
    that app and library authors implementing custom cross-contract auth
    can more easily assemble complex transactions.

Additional changes:

Copy link
Contributor

@Shaptic Shaptic left a comment

Choose a reason for hiding this comment

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

Small comment to simplify the code, then can merge!

src/contract/assembled_transaction.ts Outdated Show resolved Hide resolved
Copy link

@Ifropc Ifropc left a comment

Choose a reason for hiding this comment

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

@chadoh would you mind taking a look at George's comments? I believe we are super close to merging this one.

test/e2e/wasms/README.md Outdated Show resolved Hide resolved
src/contract/assembled_transaction.ts Outdated Show resolved Hide resolved
@chadoh chadoh force-pushed the fix/signing-by-contract branch from 5b79cce to a3983e5 Compare September 25, 2024 21:47
test/e2e/initialize.sh Outdated Show resolved Hide resolved
@chadoh
Copy link
Contributor Author

chadoh commented Sep 25, 2024

Updated with new test cases from https://github.com/AhaLabs/soroban-test-examples, "cloning" it with https://github.com/Rich-Harris/degit. This allows us to have actual contracts that exercise the sign-via-cross-contract-call, which makes it easy/possible to test sign in addition to needsNonInvokerSigningBy, and makes it a real end-to-end test.

@chadoh chadoh force-pushed the fix/signing-by-contract branch from a3983e5 to 7b3ed2d Compare September 26, 2024 13:20
@chadoh chadoh force-pushed the fix/signing-by-contract branch 3 times, most recently from 2daca79 to 9d74e11 Compare September 26, 2024 13:47
test/e2e/initialize.sh Outdated Show resolved Hide resolved
src/contract/assembled_transaction.ts Show resolved Hide resolved
@chadoh chadoh marked this pull request as draft September 26, 2024 19:31
@chadoh
Copy link
Contributor Author

chadoh commented Sep 26, 2024

Marked as draft again. Working with @kalepail today to figure out how to make signAuthEntries work for cross-contract auth, too. Tests & code incoming.

@chadoh chadoh force-pushed the fix/signing-by-contract branch 4 times, most recently from 2d805c3 to 236752a Compare September 26, 2024 23:07
@chadoh chadoh marked this pull request as ready for review September 26, 2024 23:08
@chadoh chadoh force-pushed the fix/signing-by-contract branch from 236752a to 6c7692d Compare September 26, 2024 23:10
@chadoh
Copy link
Contributor Author

chadoh commented Sep 26, 2024

Ok, ready for review! This now introduces a breaking change. I've updated the description with lots of info.

@chadoh chadoh changed the title fix: nonInvokerSigningBy contracts feat!: enable cross-contract auth Sep 27, 2024
Copy link

@Ifropc Ifropc left a comment

Choose a reason for hiding this comment

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

LGTM.
Adding a authorizeEntry customization looks a bit premature to me, but I'm by no mean a js sdk exert

@chadoh
Copy link
Contributor Author

chadoh commented Sep 27, 2024

Adding a authorizeEntry customization looks a bit premature to me, but I'm by no mean a js sdk exert

This allows @kalepail to write some really slick code

@Ifropc
Copy link

Ifropc commented Sep 27, 2024

Is it to custom-sign the contract authes?
I feel like we def need to document that...

Copy link
Contributor

@Shaptic Shaptic left a comment

Choose a reason for hiding this comment

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

A few minor comments/questions, but otherwise this LGTM and can basically be merged once those are resolved

src/contract/assembled_transaction.ts Show resolved Hide resolved
src/contract/assembled_transaction.ts Show resolved Hide resolved
src/contract/assembled_transaction.ts Show resolved Hide resolved
test/e2e/initialize.sh Outdated Show resolved Hide resolved
test/e2e/src/test-non-invoker-signing-by-contracts.js Outdated Show resolved Hide resolved
Copy link
Contributor

@Shaptic Shaptic left a comment

Choose a reason for hiding this comment

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

I'll be OOO next week and I don't want to block the merge, so I'll ✔️ but please make sure to consider the submodule approach as well as ensuring this is clearly denoted in the changelog!

@kalepail
Copy link
Contributor

kalepail commented Oct 1, 2024

Would love to see this finished, merged and a new release cut this week if possible. Lots of my work is riding on this getting merged.

@chadoh chadoh force-pushed the fix/signing-by-contract branch from 2c483d4 to d8682d9 Compare October 2, 2024 15:12
@chadoh chadoh force-pushed the fix/signing-by-contract branch from d8682d9 to ba2823e Compare October 2, 2024 15:31
@chadoh
Copy link
Contributor Author

chadoh commented Oct 2, 2024

Is it to custom-sign the contract authes?
I feel like we def need to document that...

Good point, @Ifropc. Not sure I understand the actual use-case well enough to explain the possibilities, but I'll fill in what I can, and we can think about where to add more documentation.

signAuthEntries is used in multi-auth workflows to sign all auth entries belonging to a particular address. You need to pass in signAuthEntry, which is a standard part of wallet interfaces; see https://github.com/stellar/stellar-protocol/blob/master/ecosystem/sep-0043.md#wallet-interface-format. @kalepail convinced me that signAuthEntry is poorly named, since what it actually does is sign a preimage of an auth entry. Here's the relevant parts of the authorizeEntry implementation in stellar base:

export async function authorizeEntry(
  entry,
  signer,
  validUntilLedgerSeq,
  networkPassphrase = Networks.FUTURENET
) {
  const preimage = xdr.HashIdPreimage.envelopeTypeSorobanAuthorization(
    new xdr.HashIdPreimageSorobanAuthorization({
      networkId: hash(Buffer.from(networkPassphrase)),
      nonce: addrAuth.nonce(),
      invocation: clone.rootInvocation(),
      signatureExpirationLedger: addrAuth.signatureExpirationLedger()
    })
  );
  const payload = hash(preimage.toXDR());

  let signature;
  let publicKey;
  if (typeof signer === 'function') {
    signature = Buffer.from(await signer(preimage));
    publicKey = Address.fromScAddress(addrAuth.address()).toString();
  } else {
    signature = Buffer.from(signer.sign(payload));
    publicKey = signer.publicKey();
  }

Regardless of the future of SEP-43 and the possibility of renaming signAuthEntry to something like signAuthEntryPreimage, it will never be the right fit for cross-contract auth workflows. For assembling transactions that use cross-contract auth, you need to to completely customize authorizeEntry, rather than relying on the implementation from stellar-base shown above.

I'd love to understand exactly how you used this authorizeEntry override, @kalepail. I searched https://github.com/kalepail/passkey-kit to get a sense of it, but honestly it's so slick that it just sorta looks unimpressive.

image

Would you be able to fill in some details on how you accomplish your cross-contract auth workflow using this new assembleTransaction option? We could also wait for your Meridian talk in two weeks, if you're going to cover it then 😄

@chadoh chadoh force-pushed the fix/signing-by-contract branch 4 times, most recently from aafe0e7 to b4e207d Compare October 3, 2024 15:24
Fixes: stellar#1030

Soroban allows contract authors to call `require_auth()` not only on a
G-address (for users), but also on a C-address (for contracts). This
will result in a cross-contract call to the `__check_auth` function in
the signing contract. This enables all sorts of powerful and novel
use-cases. See https://developers.stellar.org/docs/build/guides/conventions/check-auth-tutorials#customaccountinterface-implementation

Just as if one account submits a transaction (meaning that account signs
the transaction envelope) but the contract calls `require_auth` on a
different one, the app author will need to use
`needsNonInvokerSigningBy` and `signAuthEntries`, so app authors benefit
from these functions when `require_auth` is called on a contract.

This fixes various assumptions that these functions made to also work
with this sort of cross-contract auth.

- `needsNonInvokerSigningBy` now returns contract addresses
- `sign` ignores contracts in the `needsNonInvokerSigningBy` list, since
  the actual authorization will happen via cross-contract call
- `signAuthEntry` now takes an `address` instead of a `publicKey`, since
  this address may be a user's public key (a G-address) or a contract
  address. Furthermore, it allows setting a custom `authorizeEntry`, so
  that app and library authors implementing custom cross-contract auth
  can more easily assemble complex transactions.

Additional changes:

- switch to new test cases from AhaLabs/soroban-test-contracts, embed as
  git submodule
- switch to `stellar` instead of `soroban`
- use latest cli version (this version fixes a problem that some of the
  tests were working around, so they were removed)
- add (untested) workaround for stellar#1070
@chadoh chadoh force-pushed the fix/signing-by-contract branch from b4e207d to 02261d9 Compare October 3, 2024 15:24
@kalepail
Copy link
Contributor

kalepail commented Oct 3, 2024

Is it to custom-sign the contract authes? I feel like we def need to document that...

See this comment here: #1044 (comment)

It's a little bit of a tricky thing to document specifically as it's essentially allowing the developer to decide how they want to handle modifications to the auth entry. In the custom account model this is essential as __check_auth allows the ability to configure a developer defined signature type which will require the ability for the JS SDK to custom craft the signature for the auth entry. Hence the need to override what the current default does which is assume a G-address non-invoker signer.

@kalepail
Copy link
Contributor

kalepail commented Oct 3, 2024

@chadoh

I'd love to understand exactly how you used this authorizeEntry override, @kalepail.

The latest and greatest are currently in the next branch.

https://github.com/kalepail/passkey-kit/blob/next/src/kit.ts#L183-L192

Then used here: https://github.com/kalepail/passkey-kit/blob/next/src/kit.ts#L387-L393

@kalepail kalepail merged commit ebab183 into stellar:master Oct 3, 2024
11 checks passed
@chadoh chadoh deleted the fix/signing-by-contract branch October 3, 2024 15:45
@Shaptic
Copy link
Contributor

Shaptic commented Oct 8, 2024

Just for the record, I wanted to mention something here:

@kalepail convinced me that signAuthEntry is poorly named, since what it actually does is sign a preimage of an auth entry.

Nobody knows what a preimage is (which is a stupid mathy gatekeeping term for "hash function input" or "thing that got hashed"), and they shouldn't need to. The signAuthEntry method gives you a signature to insert into the authorization entry which represents, or otherwise put it returns the correct signature for that auth entry, so it is the defacto way to... sign auth entries, lol.

To your point about building custom auth structures, that would have been slightly easier with the previous helper functions, but I totally agree that we could extend authorizeEntry with a buildAuthStructure parameter or what-have-you that returns the actual ScVal needed for signatureArgs. It's a good idea! But these helpers were never made with custom auth in mind.

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.

AssembledTransaction.needsNonInvokerSigningBy() assumes all auth addresses are stellar accounts
5 participants