fix: nonInvokerSigningBy contracts

Fixes: #1030

> `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.

also:

- switch to new test cases from AhaLabs/soroban-test-contracts
- 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

.cargo/config.toml

@@ -1,7 +1,7 @@
 # paths = ["/path/to/override"] # path dependency overrides
 
 [alias] # command aliases
-install_soroban = "install --version 21.4.1 --root ./target soroban-cli --debug"
+install_stellar = "install --version 21.5.0 --root ./target stellar-cli --debug --force"
 # b = "build --target wasm32-unknown-unknown --release"
 # c = "check"
 # t = "test"

.env

@@ -1,4 +1,4 @@
-SOROBAN_NETWORK_PASSPHRASE="Standalone Network ; February 2017"
-SOROBAN_RPC_URL="http://localhost:8000/soroban/rpc"
-SOROBAN_ACCOUNT="me"
+STELLAR_NETWORK_PASSPHRASE="Standalone Network ; February 2017"
+STELLAR_RPC_URL="http://localhost:8000/soroban/rpc"
+STELLAR_ACCOUNT="me"
 FRIENDBOT_URL="http://localhost:8000/friendbot"

src/contract/assembled_transaction.ts

@@ -2,6 +2,7 @@
 /* eslint max-classes-per-file: 0 */
 import {
   Account,
+  Address,
   BASE_FEE,
   Contract,
   Operation,
@@ -636,9 +637,11 @@ export class AssembledTransaction<T> {
       );
     }
 
-    if (this.needsNonInvokerSigningBy().length) {
+    // filter out contracts, as these are dealt with via cross contract calls
+    const sigsNeeded = this.needsNonInvokerSigningBy().filter(id => !id.startsWith('C'));
+    if (sigsNeeded.length) {
       throw new AssembledTransaction.Errors.NeedsMoreSignatures(
-        "Transaction requires more signatures. " +
+        `Transaction requires signatures from ${sigsNeeded}. ` +
         "See `needsNonInvokerSigningBy` for details.",
       );
     }
@@ -760,9 +763,9 @@ export class AssembledTransaction<T> {
                 "scvVoid"),
           )
           .map((entry) =>
-            StrKey.encodeEd25519PublicKey(
-              entry.credentials().address().address().accountId().ed25519(),
-            ),
+            Address.fromScAddress(
+              entry.credentials().address().address(),
+            ).toString(),
           ),
       ),
     ];

test/e2e/initialize.sh

@@ -14,50 +14,51 @@ dirname="$(CDPATH= cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
 
 echo "###################### Initializing e2e tests ########################"
 
-soroban="$dirname/../../target/bin/soroban"
-if [[ -f "$soroban" ]]; then
-  current=$($soroban --version | head -n 1 | cut -d ' ' -f 2)
+stellar=$(realpath "$dirname/../../target/bin/stellar")
+if [[ -f "$stellar" ]]; then
+  current=$($stellar --version | head -n 1 | cut -d ' ' -f 2)
   desired=$(cat .cargo/config.toml | grep -oE -- "--version\s+\S+" | awk '{print $2}')
   if [[ "$current" != "$desired" ]]; then
-    echo "Current pinned soroban binary: $current. Desired: $desired. Building soroban binary."
-    (cd "$dirname/../.." && cargo install_soroban)
+    echo "Current pinned stellar binary: $current. Desired: $desired. Building stellar binary."
+    (cd "$dirname/../.." && cargo install_stellar)
   else
-    echo "Using soroban binary from ./target/bin"
+    echo "Using stellar binary from ./target/bin"
   fi
 else
-  echo "Building pinned soroban binary"
-  (cd "$dirname/../.." && cargo install_soroban)
+  echo "Building pinned stellar binary"
+  (cd "$dirname/../.." && cargo install_stellar)
 fi
 
-NETWORK_STATUS=$(curl -s -X POST "$SOROBAN_RPC_URL" -H "Content-Type: application/json" -d '{ "jsonrpc": "2.0", "id": 8675309, "method": "getHealth" }' | sed -n 's/.*"status":\s*"\([^"]*\)".*/\1/p')
+NETWORK_STATUS=$(curl -s -X POST "$STELLAR_RPC_URL" -H "Content-Type: application/json" -d '{ "jsonrpc": "2.0", "id": 8675309, "method": "getHealth" }' | sed -n 's/.*"status":\s*"\([^"]*\)".*/\1/p')
 
 echo Network
-echo "  RPC:        $SOROBAN_RPC_URL"
-echo "  Passphrase: \"$SOROBAN_NETWORK_PASSPHRASE\""
+echo "  RPC:        $STELLAR_RPC_URL"
+echo "  Passphrase: \"$STELLAR_NETWORK_PASSPHRASE\""
 echo "  Status:     $NETWORK_STATUS"
 
 if [[ "$NETWORK_STATUS" != "healthy" ]]; then
   echo "Network is not healthy (not running?), exiting"
   exit 1
 fi
 
-$soroban keys generate $SOROBAN_ACCOUNT
+$stellar keys generate $STELLAR_ACCOUNT
 
-# retrieve the contracts using soroban contract init then build them if they dont already exist
+# retrieve the contracts using stellar contract init then build them if they dont already exist
 # Define directory and WASM file paths
 target_dir="$dirname/test-contracts/target/wasm32-unknown-unknown/release"
 contracts_dir="$dirname/test-contracts"
-repo_url="https://github.com/stellar/soroban-examples.git"
+test_examples_repo="https://github.com/AhaLabs/soroban-test-examples.git"
 wasm_files=(
-    "soroban_other_custom_types_contract.wasm"
-    "soroban_atomic_swap_contract.wasm"
-    "soroban_token_contract.wasm"
-    "soroban_increment_contract.wasm"
-    "hello_world.wasm"
+    "custom_types.wasm"
+    "atomic_swap.wasm"
+    "token.wasm"
+    "increment.wasm"
+    "needs_a_signature.wasm"
+    "this_one_signs.wasm"
 )
 
 get_remote_git_hash() {
-    git ls-remote "$repo_url" HEAD | cut -f1
+    git ls-remote "$test_examples_repo" HEAD | cut -f1
 }
 
 # Get the current git hash
@@ -75,7 +76,7 @@ fi
 all_exist=true
 for wasm_file in "${wasm_files[@]}"; do
     if [ ! -f "$target_dir/$wasm_file" ]; then
-        all_exist=false
+    all_exist=false
         break
     fi
 done
@@ -84,11 +85,11 @@ done
 if [ "$all_exist" = false ] || [ "$current_hash" != "$stored_hash" ]; then
     echo "WASM files are missing or contracts have been updated. Initializing and building contracts..."
     # Initialize contracts
-    $soroban contract init "$dirname/test-contracts" --with-example increment other_custom_types atomic_swap token
-    
+    npx degit --force AhaLabs/soroban-test-examples "$dirname/test-contracts"
+
     # Change directory to test-contracts and build the contracts
     cd "$dirname/test-contracts" || { echo "Failed to change directory!"; exit 1; }
-    $soroban contract build
+    $stellar contract build
     # Save git hash to file
     echo "$current_hash" > "$hash_file"
 else

test/e2e/src/test-contract-client-constructor.js

@@ -5,6 +5,7 @@ const {
   networkPassphrase,
   rpcUrl,
   generateFundedKeypair,
+  run,
 } = require("./util");
 const { Address, contract } = require("../../../lib");
 
@@ -15,47 +16,32 @@ async function clientFromConstructor(
   if (!contracts[name]) {
     throw new Error(
       `Contract ${name} not found. ` +
-        `Pick one of: ${Object.keys(contracts).join(", ")}`,
+        `Pick one of: ${Object.keys(contracts).join()}`,
     );
   }
   keypair = await keypair; // eslint-disable-line no-param-reassign
   const wallet = contract.basicNodeSigner(keypair, networkPassphrase);
 
   const { path } = contracts[name];
-  const xdr = JSON.parse(
-    spawnSync(
-      "./target/bin/soroban",
-      ["contract", "inspect", "--wasm", path, "--output", "xdr-base64-array"],
-      { shell: true, encoding: "utf8" },
-    ).stdout.trim(),
-  );
+  const inspected = run(
+    `./target/bin/stellar contract inspect --wasm ${path} --output xdr-base64-array`,
+  ).stdout;
+  const xdr = JSON.parse(inspected);
 
   const spec = new contract.Spec(xdr);
   let wasmHash = contracts[name].hash;
   if (!wasmHash) {
-    wasmHash = spawnSync(
-      "./target/bin/soroban",
-      ["contract", "install", "--wasm", path],
-      { shell: true, encoding: "utf8" },
-    ).stdout.trim();
+    wasmHash = run(
+      `./target/bin/stellar contract install --wasm ${path}`,
+    ).stdout;
   }
 
   // TODO: do this with js-stellar-sdk, instead of shelling out to the CLI
   contractId =
     contractId ??
-    spawnSync(
-      "./target/bin/soroban",
-      [
-        // eslint-disable-line no-param-reassign
-        "contract",
-        "deploy",
-        "--source",
-        keypair.secret(),
-        "--wasm-hash",
-        wasmHash,
-      ],
-      { shell: true, encoding: "utf8" },
-    ).stdout.trim();
+    run(
+      `./target/bin/stellar contract deploy --source ${keypair.secret()} --wasm-hash ${wasmHash}`,
+    ).stdout;
 
   const client = new contract.Client(spec, {
     networkPassphrase,
@@ -92,15 +78,15 @@ async function clientForFromTest(contractId, publicKey, keypair) {
 describe("Client", function () {
   before(async function () {
     const { client, keypair, contractId } =
-      await clientFromConstructor("helloWorld");
+      await clientFromConstructor("customTypes");
     const publicKey = keypair.publicKey();
     const addr = Address.fromString(publicKey);
     this.context = { client, publicKey, addr, contractId, keypair };
   });
 
   it("can be constructed with `new Client`", async function () {
-    const { result } = await this.context.client.hello({ to: "tests" });
-    expect(result).to.deep.equal(["Hello", "tests"]);
+    const { result } = await this.context.client.hello({ hello: "tests" });
+    expect(result).to.equal("tests");
   });
 
   it("can be constructed with `from`", async function () {

test/e2e/src/test-methods-as-args.js

@@ -8,8 +8,8 @@ function callMethod(method, args) {
 
 describe("methods-as-args", function () {
   it("should pass methods as arguments and have them still work", async function () {
-    const { client } = await clientFor("helloWorld");
-    const { result } = await callMethod(client.hello, { to: "tests" });
-    expect(result).to.deep.equal(["Hello", "tests"]);
+    const { client } = await clientFor("customTypes");
+    const { result } = await callMethod(client.hello, { hello: "tests" });
+    expect(result).to.equal("tests");
   });
 });

test/e2e/src/test-non-invoker-signing-by-contracts.js

@@ -0,0 +1,32 @@
+const { expect } = require("chai");
+const { clientFor } = require("./util");
+
+before(async function () {
+  const { contractId: signingContractId, keypair } =
+    await clientFor("doesSigning");
+  const { client: needsSignature } = await clientFor("needsSignature", {
+    keypair,
+  });
+
+  const tx = await needsSignature.hello({
+    to: keypair.publicKey(),
+    sign_with: signingContractId,
+  });
+  this.context = { tx, signingContractId };
+});
+
+describe("needsNonInvokerSigningBy", function () {
+  it("does not assume stellar accounts", async function () {
+    expect(this.context.tx.needsNonInvokerSigningBy()[0]).to.equal(
+      this.context.signingContractId,
+    );
+  });
+});
+
+describe("sign", function () {
+  it("doesn't throw error when nonInvokerSigningBy returns a contract", async function () {
+    expect(
+      async () => await this.context.tx.sign({ force: true }),
+    ).to.not.throw();
+  });
+});