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

[JakeUrban]

Describe the bug

needsNonInvokerSigningBy()'s filter() condition returns authorization entires whose address.signature value is scvVoid. In my case, this address was a contract. I was trying to figure out how to sign a transaction that used custom auth.

However, the returned list is then mapped through a function that assumes all addresses are stellar account public keys. This lead to a TypeError: accountId not set error.

To Reproduce

Use the following transaction to construct an AssembledTransaction and call needsNonInvokerSigningBy():

AAAAAgAAAADhY+75HJcYjxA07MhDzZk/DwQzVITe9slCwDgcEGcc+AACfJ8AEJEBAAAAAwAAAAEAAAAAAAAAAAAAAABmv+l8AAAAAAAAAAEAAAAAAAAAGAAAAAAAAAABOSACpv7RbyYK4XnWgj9AJ/GjrIPjLEoWJa/Iqo1a//YAAAAEbWludAAAAAIAAAASAAAAAAAAAADhY+75HJcYjxA07MhDzZk/DwQzVITe9slCwDgcEGcc+AAAAAoAAAAAAAAAAAAAAAAAAABkAAAAAQAAAAEAAAABEGJ2U3ldj9dca1n1NK0i2XkfL/5zXZsxAOIxUmn9RWknYhAjSWxRgQAAAAAAAAABAAAAAAAAAAE5IAKm/tFvJgrhedaCP0An8aOsg+MsShYlr8iqjVr/9gAAAARtaW50AAAAAgAAABIAAAAAAAAAAOFj7vkclxiPEDTsyEPNmT8PBDNUhN72yULAOBwQZxz4AAAACgAAAAAAAAAAAAAAAAAAAGQAAAAAAAAAAQAAAAAAAAAEAAAAAAAAAAAagL+zRaDmKTHWcB67aFNUZNLVg6ojcyrbO1u3Sg/KcAAAAAYAAAABOSACpv7RbyYK4XnWgj9AJ/GjrIPjLEoWJa/Iqo1a//YAAAAUAAAAAQAAAAYAAAABwfs5RVshthImbX+wmqWS0t1AjL4y5EjMMtSk/oEy11AAAAAUAAAAAQAAAAeheXOfVamUQyN7XuqlXPfXnmxx6tJxnGFVIA2YxmGnMwAAAAIAAAABAAAAAOFj7vkclxiPEDTsyEPNmT8PBDNUhN72yULAOBwQZxz4AAAAAlRFU1Q0AAAAAAAAAAAAAAAagL+zRaDmKTHWcB67aFNUZNLVg6ojcyrbO1u3Sg/KcAAAAAYAAAABEGJ2U3ldj9dca1n1NK0i2XkfL/5zXZsxAOIxUmn9RWkAAAAVJ2IQI0lsUYEAAAAAABUmowAADXgAAADEAAAAAAACfDsAAAAA

Expected behavior

The function should not assume all auth entries addresses are stellar accounts -- it should also support contract addresses.

[JakeUrban]

Looking at the implementation of signAuthEntries(), it appears as if we'll also need to update the function to support passing contract addresses. Right now it, as well as the stellar-base function it calls, authorizeEntry(), only supports signing with stellar accounts.

[JakeUrban]

@leighmcculloch I'm curious on your take here -- we should be able to support auth entries that need signatures from signers of contracts right? The SDK doesn't need to know what auth scheme the contract uses.

[Shaptic]

I’m confused as to what it means for a contract to sign for something when it doesn’t have a private key.

[JakeUrban]

In short, if a contract (A) calls require_auth() on an Address, and that address is a contract (B), the host will call B.__check_auth() and pass it the authorization entries and context for the transaction. The contract can then apply arbitrary rules and ultimately return true/false to authorize/fail the transaction. The rules it implements could be that one or more entries have to have a signature from a particular key it tracks in its storage, or it could something like "you can only call this contract every X ledgers".

This is what we've called custom auth or a custom account as an admin on the contract in our docs.

[leighmcculloch]

@leighmcculloch I'm curious on your take here -- we should be able to support auth entries that need signatures from signers of contracts right? The SDK doesn't need to know what auth scheme the contract uses.

+1. The JS SDK should support auth entries for custom auth. The SDK won't be able to produce the signatures, but it should allow setting them.

I’m confused as to what it means for a contract to sign for something when it doesn’t have a private key.

In custom auth the contract doesn't sign for the auth, the contract validates a signature passed in. The signature can be whatever the contract expects. It could even be no signature in the event the custom auth is gating behavior based on other inputs such as the context of the call (call stack, arguments, etc).

[JakeUrban]

Hey all, what is the status of this?

[chadoh]

I've fixed the nonInvokerSigningBy return in #1044, but didn't yet alter the behavior of signAuthEntries. I'm not totally sure the correct way to handle it, yet.

I love the WebAuthn tutorial you linked to, @JakeUrban, though I haven't gone the whole way through it yet. I wonder if we should turn that into an examples in https://github.com/stellar/soroban-examples, then create a real e2e test for it here (the test I added in #1044 is in the e2e folder, but it's not actually an end-to-end test).

[JakeUrban]

I'm in favor of turning it into an example in the repo.

I do think updating signAuthEntries() and authorizeEntry() to support signatures from signers of contract accounts is in the critical path to smart wallet adoption though, so I think this should be prioritized. cc @janewang