-
Notifications
You must be signed in to change notification settings - Fork 314
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
Conversation
There was a problem hiding this 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!
There was a problem hiding this 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.
5b79cce
to
a3983e5
Compare
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 |
a3983e5
to
7b3ed2d
Compare
2daca79
to
9d74e11
Compare
Marked as draft again. Working with @kalepail today to figure out how to make |
2d805c3
to
236752a
Compare
236752a
to
6c7692d
Compare
Ok, ready for review! This now introduces a breaking change. I've updated the description with lots of info. |
There was a problem hiding this 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
6c7692d
to
2c483d4
Compare
This allows @kalepail to write some really slick code |
Is it to custom-sign the contract authes? |
There was a problem hiding this 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
There was a problem hiding this 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!
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. |
2c483d4
to
d8682d9
Compare
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected] |
d8682d9
to
ba2823e
Compare
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.
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 I'd love to understand exactly how you used this Would you be able to fill in some details on how you accomplish your cross-contract auth workflow using this new |
aafe0e7
to
b4e207d
Compare
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
b4e207d
to
02261d9
Compare
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 |
The latest and greatest are currently in the 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 |
Just for the record, I wanted to mention something here:
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 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 |
Fixes: #1030
Soroban allows contract authors to call
require_auth()
not only on aG-address (for users), but also on a C-address (for contracts). This
will result in a cross-contract call to the
__check_auth
function inthe 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 adifferent one, the app author will need to use
needsNonInvokerSigningBy
andsignAuthEntries
, so app authors benefitfrom 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 addressessign
ignores contracts in theneedsNonInvokerSigningBy
list, sincethe actual authorization will happen via cross-contract call
signAuthEntry
now takes anaddress
instead of apublicKey
, sincethis address may be a user's public key (a G-address) or a contract
address. Furthermore, it allows setting a custom
authorizeEntry
, sothat app and library authors implementing custom cross-contract auth
can more easily assemble complex transactions.
Additional changes:
stellar
instead ofsoroban
tests were working around, so they were removed)