search

obsidiansystems / ledger-app-nervos

MIT License
10 stars 10 forks source link

[LEDGERJS] Documentation and demo code on signTransaction #161

Closed katat closed 3 years ago

katat commented 4 years ago

I have been trying to sign a transaction using ledgerjs and send it to the transaction pool to verify the whole process works.

With the signing pattern https://github.com/obsidiansystems/ledger-app-nervos/issues/145#issuecomment-672887250, I am using the ckb-sdk-js example to facilitate generating a raw transaction with the unspent cells as well as fetching the context transaction, and modify it to sign with ledgerjs. To keep it simple, the script only involves one input and one address for both sender and recipient, so there is only one signature to sign.

But somehow the node RPC throws Error: {"code":-3,"message":"Script: ValidationFailure(-2)”}. Not sure if this is relating to the address issue as reported above. Also, I noticed there are discrepancies in some of the data formats between the annotation transaction and the node RPC specs, such as the capacity (https://github.com/nervosnetwork/ckb/blob/cdb73dc5bbd09b4edc87f2dd2e3b40cfa73c7ab9/rpc/json/rpc.json#L633) and since (https://github.com/nervosnetwork/ckb/blob/cdb73dc5bbd09b4edc87f2dd2e3b40cfa73c7ab9/rpc/json/rpc.json#L628). I have to convert these fields to match with the format that ledgerjs required. Even though, the resulting signature doesn’t seem to work when going through the validation process on the CKB node.

I know you have already provided an example for signing a transaction. To help debug the issue, could you please create a sample script based on the CKB SDK example(https://github.com/nervosnetwork/ckb-sdk-js/blob/3152a3c30da40fead8a18aee43da2eca94cd1347/packages/ckb-sdk-core/examples/sendTransactionWithLumosCollector.js) to verify the whole process, generating transaction/signing transaction/sending transaction to the pool, with ledgerjs is actually working?

Besides, I think we need to improve documentation on how to use the signTransaction, such as how to organize the resulting signature in a raw transaction and how to sign for different paths for a raw transaction. In my view, these operations are not trivial and should be better documented. If the documentation can be done through reproducible integration tests, it would be even better.

Besides, I would suggest renaming the signTransaction as described in https://github.com/obsidiansystems/ledger-app-nervos/issues/160.

mikereinhart commented 4 years ago

Hi @katat, this is being addressed in https://github.com/obsidiansystems/ledgerjs/pull/26

mikereinhart commented 4 years ago

https://github.com/obsidiansystems/ledgerjs/pull/26 has been merged!

katat commented 4 years ago

Thanks @mikereinhart I had some tests to run the demo script and it works for transactions involving only one sign path. When I tried to sign a transaction having two inputs by different sign paths following the https://github.com/obsidiansystems/ledger-app-nervos/issues/145#issuecomment-672887250, the ledger throws error. The signing code is like below:

await ledger.signTransaction("44'/309'/0'/0/0", rawTx, rawTx.witnesses, formattedCtxd, "44'/309'/0'/0/0")
await ledger.signTransaction("44'/309'/0'/0/1", rawTx, rawTx.witnesses, formattedCtxd, "44'/309'/0'/0/0")

The transaction can be signed with path 44'/309'/0'/0/0, but throws the following error when trying to sign with path 44'/309'/0'/0/1.

Ledger device: Condition of use not satisfied (denied by the user?) (0x6985)

Any ideas on this error?

Raw transaction to sign:

{
  "version": "0x0",
  "cellDeps": [
    {
      "outPoint": {
        "txHash": "0x108d72f9213b7146298a318564b5405341dd0f370df4c0a435c79ea7835d77a4",
        "index": "0x0"
      },
      "depType": "depGroup"
    }
  ],
  "headerDeps": [],
  "inputs": [
    {
      "previousOutput": {
        "txHash": "0xce11d3e73cd7f1ba4a5f262f25c76ff6403003765e84301a858daabbf8ff3ba0",
        "index": "0x0"
      },
      "since": "0x0"
    },
    {
      "previousOutput": {
        "txHash": "0xdd9f3d3a8fc9f8fab30af9ff9ba1992a9de9aafe95b8079da48cb61ba762876c",
        "index": "0x0"
      },
      "since": "0x0"
    }
  ],
  "outputs": [
    {
      "capacity": "0x2540be400",
      "lock": {
        "codeHash": "0x9bd7e06f3ecf4be0f2fcd2188b23f1b9fcc88e5d4b65a8637b17723bbda3cce8",
        "hashType": "type",
        "args": "0xddbd7f09eb480450c1b1ed2c8696248de91c6802"
      }
    },
    {
      "capacity": "0x2540a5d60",
      "lock": {
        "codeHash": "0x9bd7e06f3ecf4be0f2fcd2188b23f1b9fcc88e5d4b65a8637b17723bbda3cce8",
        "hashType": "type",
        "args": "0xb06a8695d6908fec7d2729f6bc34a3086b091e3f"
      }
    }
  ],
  "witnesses": [],
  "outputsData": [
    "0x",
    "0x"
  ]
}

Context transaction:

[
  {
    "cellDeps": [
      {
        "outPoint": {
          "txHash": "0x108d72f9213b7146298a318564b5405341dd0f370df4c0a435c79ea7835d77a4",
          "index": "0x0"
        },
        "depType": "depGroup"
      }
    ],
    "inputs": [
      {
        "previousOutput": {
          "txHash": "0x0bebb5eedde97cdb7cf454b27985899fc0bc650a7ee596dcde35039868f0c982",
          "index": "0xe"
        },
        "since": "0x0"
      }
    ],
    "outputs": [
      {
        "lock": {
          "codeHash": "0x9bd7e06f3ecf4be0f2fcd2188b23f1b9fcc88e5d4b65a8637b17723bbda3cce8",
          "hashType": "type",
          "args": "0xb06a8695d6908fec7d2729f6bc34a3086b091e3f"
        },
        "type": null,
        "capacity": "0x2540be400"
      },
      {
        "lock": {
          "codeHash": "0x9bd7e06f3ecf4be0f2fcd2188b23f1b9fcc88e5d4b65a8637b17723bbda3cce8",
          "hashType": "type",
          "args": "0xc403416bd250be3c323a89250dee78b6fabdf61e"
        },
        "type": null,
        "capacity": "0x14f46b0230"
      }
    ],
    "outputsData": [
      "0x",
      "0x"
    ],
    "headerDeps": [],
    "hash": "0xce11d3e73cd7f1ba4a5f262f25c76ff6403003765e84301a858daabbf8ff3ba0",
    "version": "0x0",
    "witnesses": [
      "0x5500000010000000550000005500000041000000af8016defa3482bf5901460ef5b4bf2e5696b4a6b71f41ca49242b77ae4ab7e25a321b179b5e5782a079752d5611986fe9c38c933ae0332f769227f6ac88621801"
    ]
  },
  {
    "cellDeps": [
      {
        "outPoint": {
          "txHash": "0x108d72f9213b7146298a318564b5405341dd0f370df4c0a435c79ea7835d77a4",
          "index": "0x0"
        },
        "depType": "depGroup"
      }
    ],
    "inputs": [
      {
        "previousOutput": {
          "txHash": "0xce11d3e73cd7f1ba4a5f262f25c76ff6403003765e84301a858daabbf8ff3ba0",
          "index": "0x1"
        },
        "since": "0x0"
      }
    ],
    "outputs": [
      {
        "lock": {
          "codeHash": "0x9bd7e06f3ecf4be0f2fcd2188b23f1b9fcc88e5d4b65a8637b17723bbda3cce8",
          "hashType": "type",
          "args": "0xb4341c5b4920989ea6b1146abcb44988628e0980"
        },
        "type": null,
        "capacity": "0x2540be400"
      },
      {
        "lock": {
          "codeHash": "0x9bd7e06f3ecf4be0f2fcd2188b23f1b9fcc88e5d4b65a8637b17723bbda3cce8",
          "hashType": "type",
          "args": "0xae6fcb80b488d6fa2b719e5e5928f5cb7f63e957"
        },
        "type": null,
        "capacity": "0x12a05f1c60"
      }
    ],
    "outputsData": [
      "0x",
      "0x"
    ],
    "headerDeps": [],
    "hash": "0xdd9f3d3a8fc9f8fab30af9ff9ba1992a9de9aafe95b8079da48cb61ba762876c",
    "version": "0x0",
    "witnesses": [
      "0x55000000100000005500000055000000410000003321e46484b1c1916752ad1ee01cd0d1e2b68707e6c94de021dcf70ea755dfcf12eaadb438e766d02e6c13f52f0471010248c7613fca89c3d62afb9773cb1a3600"
    ]
  }
]
mikereinhart commented 3 years ago

Hi @katat this is fixed in https://github.com/obsidiansystems/ledgerjs/commit/fc2c19003cc3bf46d3d0b30d266ea47434120bbc. Can you give that git hash a try?

yuche commented 3 years ago

Hi @katat this is fixed in obsidiansystems/ledgerjs@fc2c190. Can you give that git hash a try?

This fix does fix the problem. But in the previously mentioned example, the groupWitnesses parameter of the signTransaction function had to be passed as a string for the function to work, but in the type declaration of this function the groupWitnesses parameter needs to be an array of strings, and I wonder if this is expected behavior:

// It works.
rawTransaction.witnesses[0] = ckb.utils.serializeWitnessArgs({
  lock: '',
  inputType: '',
  outputType: ''
})
rawTransaction.witnesses[1] = ckb.utils.serializeWitnessArgs({
  lock: '',
  inputType: '',
  outputType: ''
})
await ledger.signTransaction("44'/309'/0'/0/0", rawTx, rawTx.witnesses[0], formattedCtxd, "44'/309'/0'/0/0")
await ledger.signTransaction("44'/309'/0'/0/1", rawTx, rawTx.witnesses[1], formattedCtxd, "44'/309'/0'/0/0")
jonored commented 3 years ago

I suspect you are hitting this defaulting line by passing a string: https://github.com/obsidiansystems/ledgerjs/blob/nervos/packages/hw-app-ckb/src/Ckb.js#L220

but you do indeed need to separate the witnesses for each lock argument, so [rawTx.witnesses[0]] and [rawTx.witnesses[1]] - otherwise the signatures are not valid. Hitting the default you lose access to the type script fields of the WitnessArgs.

yuche commented 3 years ago

I suspect you are hitting this defaulting line by passing a string: https://github.com/obsidiansystems/ledgerjs/blob/nervos/packages/hw-app-ckb/src/Ckb.js#L220

but you do indeed need to separate the witnesses for each lock argument, so [rawTx.witnesses[0]] and [rawTx.witnesses[1]] - otherwise the signatures are not valid. Hitting the default you lose access to the type script fields of the WitnessArgs.

Thanks for the quick reply. Now I've got a new problem with the same error when signing Nervos DAO deposit:

Ledger device: Condition of use not satisfied (denied by the user?) (0x6985)

This is the code I used when the error throws: https://gist.github.com/yuche/51d25dda02b9495f94a981c965d1cd5f

Raw transaction to sign:

{
  "version": "0x0",
  "cellDeps": [
    {
      "outPoint": {
        "txHash": "0xf8de3bb47d055cdf460d93a2a6e1b05f7432f9777c8c474abf4eec1d4aee5d37",
        "index": "0x0"
      },
      "depType": "depGroup"
    },
    {
      "outPoint": {
        "txHash": "0x8f8c79eb6671709633fe6a46de93c0fedc9c1b8a6527a18d3983879542635c9f",
        "index": "0x2"
      },
      "depType": "code"
    }
  ],
  "headerDeps": [],
  "inputs": [
    {
      "previousOutput": {
        "txHash": "0xb346a7a144652184ca5f4f87477ed4fe9df98df7514341eb48b469358ebc4af5",
        "index": "0x1"
      },
      "since": "0x0"
    }
  ],
  "outputs": [
    {
      "capacity": "0x26be36800",
      "lock": {
        "codeHash": "0x9bd7e06f3ecf4be0f2fcd2188b23f1b9fcc88e5d4b65a8637b17723bbda3cce8",
        "hashType": "type",
        "args": "0x8830a6e1db602cf8d37f5fd825c6a11c1ccd53a9"
      },
      "type": {
        "codeHash": "0x82d76d1b75fe2fd9a27dfbaa65a039221a380d76c926f378d3f81cf3e7e13f2e",
        "args": "0x",
        "hashType": "type"
      }
    },
    {
      "capacity": "0x90ee660dda0",
      "lock": {
        "codeHash": "0x9bd7e06f3ecf4be0f2fcd2188b23f1b9fcc88e5d4b65a8637b17723bbda3cce8",
        "hashType": "type",
        "args": "0x8830a6e1db602cf8d37f5fd825c6a11c1ccd53a9"
      }
    }
  ],
  "witnesses": [
    "0x10000000100000001000000010000000"
  ],
  "outputsData": [
    "0x0000000000000000",
    "0x"
  ]
}

Context transaction:

[
  {
    "version": "0x0",
    "cell_deps": [
      {
        "out_point": {
          "tx_hash": "0xf8de3bb47d055cdf460d93a2a6e1b05f7432f9777c8c474abf4eec1d4aee5d37",
          "index": "0x0"
        },
        "dep_type": "dep_group"
      }
    ],
    "inputs": [
      {
        "previous_output": {
          "tx_hash": "0x05aeb9036b36a4df7fac3fc322efc8522aa0ae415851644762913075d47db548",
          "index": "0x1"
        },
        "since": "0x0"
      }
    ],
    "outputs": [
      {
        "capacity": "0x2245cdc00",
        "lock": {
          "code_hash": "0x9bd7e06f3ecf4be0f2fcd2188b23f1b9fcc88e5d4b65a8637b17723bbda3cce8",
          "hash_type": "type",
          "args": "0x480c3aaaa49980af9372bf50d04566cef232c1b8"
        },
        "type": null
      },
      {
        "capacity": "0x9115245cc40",
        "lock": {
          "code_hash": "0x9bd7e06f3ecf4be0f2fcd2188b23f1b9fcc88e5d4b65a8637b17723bbda3cce8",
          "hash_type": "type",
          "args": "0x8830a6e1db602cf8d37f5fd825c6a11c1ccd53a9"
        },
        "type": null
      }
    ],
    "outputs_data": [
      "0x",
      "0x"
    ],
    "header_deps": [],
    "hash": "0xb346a7a144652184ca5f4f87477ed4fe9df98df7514341eb48b469358ebc4af5",
    "witnesses": [
 "0x5500000010000000550000005500000041000000d7e091a21095a91bcce3bd114d4081126195e9b15127c7413c20025367e4507d266914325f4d33728762e323c32266f7665066b2fe6e675c774f69513a2192fc00"
    ]
  }
]
yuche commented 3 years ago

I tested a variety of interactions that LedgerJS might have with Neuron:

Regular transfer

Regular transfer transactions go smoothly, both single address wallet and HD wallet work fine in various situations. Except in the case of specifying changePath:

// error
await lckb.signTransaction("44'/309'/0'/0/0", formatted, [formatted.witnesses[0]], formattedCtxd, "44'/309'/0'/1/0")
await lckb.signTransaction("44'/309'/0'/0/1", formatted, [formatted.witnesses[1]], formattedCtxd, "44'/309'/0'/1/0")
// works
await lckb.signTransaction("44'/309'/0'/0/0", formatted, [formatted.witnesses[0]], formattedCtxd, "44'/309'/0'/0/0")
await lckb.signTransaction("44'/309'/0'/0/1", formatted, [formatted.witnesses[1]], formattedCtxd, "44'/309'/0'/0/0")

Reproduction:

https://gist.github.com/yuche/b609442c55944110329a2387963d0df6

Error:

Ledger device: Condition of use not satisfied (denied by the user?) (0x6985)

Raw transaction to sign:

{
  "version": "0x0",
  "cell_deps": [
    {
      "out_point": {
        "tx_hash": "0xf8de3bb47d055cdf460d93a2a6e1b05f7432f9777c8c474abf4eec1d4aee5d37",
        "index": "0x0"
      },
      "dep_type": "dep_group"
    }
  ],
  "inputs": [
    {
      "previous_output": {
        "tx_hash": "0xb346a7a144652184ca5f4f87477ed4fe9df98df7514341eb48b469358ebc4af5",
        "index": "0x1"
      },
      "since": "0x0"
    }
  ],
  "outputs": [
    {
      "capacity": "0x9502f9000",
      "lock": {
        "code_hash": "0x9bd7e06f3ecf4be0f2fcd2188b23f1b9fcc88e5d4b65a8637b17723bbda3cce8",
        "hash_type": "type",
        "args": "0x480c3aaaa49980af9372bf50d04566cef232c1b8"
      },
      "type": null
    },
    {
      "capacity": "0x9080214b5a0",
      "lock": {
        "code_hash": "0x9bd7e06f3ecf4be0f2fcd2188b23f1b9fcc88e5d4b65a8637b17723bbda3cce8",
        "hash_type": "type",
        "args": "0x8830a6e1db602cf8d37f5fd825c6a11c1ccd53a9"
      },
      "type": null
    }
  ],
  "outputs_data": [
    "0x",
    "0x"
  ],
  "header_deps": [],
  "witnesses": [
    "0x10000000100000001000000010000000",
    "0x10000000100000001000000010000000"
  ]
}

Context transaction:

[
  {
    "version": "0x0",
    "cell_deps": [
      {
        "out_point": {
          "tx_hash": "0xf8de3bb47d055cdf460d93a2a6e1b05f7432f9777c8c474abf4eec1d4aee5d37",
          "index": "0x0"
        },
        "dep_type": "dep_group"
      }
    ],
    "inputs": [
      {
        "previous_output": {
          "tx_hash": "0x05aeb9036b36a4df7fac3fc322efc8522aa0ae415851644762913075d47db548",
          "index": "0x1"
        },
        "since": "0x0"
      }
    ],
    "outputs": [
      {
        "capacity": "0x2245cdc00",
        "lock": {
          "code_hash": "0x9bd7e06f3ecf4be0f2fcd2188b23f1b9fcc88e5d4b65a8637b17723bbda3cce8",
          "hash_type": "type",
          "args": "0x480c3aaaa49980af9372bf50d04566cef232c1b8"
        },
        "type": null
      },
      {
        "capacity": "0x9115245cc40",
        "lock": {
          "code_hash": "0x9bd7e06f3ecf4be0f2fcd2188b23f1b9fcc88e5d4b65a8637b17723bbda3cce8",
          "hash_type": "type",
          "args": "0x8830a6e1db602cf8d37f5fd825c6a11c1ccd53a9"
        },
        "type": null
      }
    ],
    "outputs_data": [
      "0x",
      "0x"
    ],
    "header_deps": [],
    "hash": "0xb346a7a144652184ca5f4f87477ed4fe9df98df7514341eb48b469358ebc4af5",
    "witnesses": [
      "0x5500000010000000550000005500000041000000d7e091a21095a91bcce3bd114d4081126195e9b15127c7413c20025367e4507d266914325f4d33728762e323c32266f7665066b2fe6e675c774f69513a2192fc00"
    ]
  }
]

DAO

Other than the previously mentioned DAO Deposit transaction, I haven't been able to test Withdraw and Unlock since I haven't been able to deposit successfully.

sUDT

There was a problem when creating a SUDT account transaction, and the returned error is the same as the DAO error:

Ledger device: Condition of use not satisfied (denied by the user?) (0x6985)

Raw transaction to sign:

{
  "version": "0x0",
  "cell_deps": [
    {
      "out_point": {
        "tx_hash": "0xf8de3bb47d055cdf460d93a2a6e1b05f7432f9777c8c474abf4eec1d4aee5d37",
        "index": "0x0"
      },
      "dep_type": "dep_group"
    },
    {
      "out_point": {
        "tx_hash": "0xc1b2ae129fad7465aaa9acc9785f842ba3e6e8b8051d899defa89f5508a77958",
        "index": "0x0"
      },
      "dep_type": "code"
    }
  ],
  "inputs": [
    {
      "previous_output": {
        "tx_hash": "0x02510e8042bb4d53b10256dcb3cd1939a3c86239b497f0e7334cf78dc513f9ad",
        "index": "0x0"
      },
      "since": "0x0"
    }
  ],
  "outputs": [
    {
      "capacity": "0x16b969d00",
      "lock": {
        "code_hash": "0x86a1c6987a4acbe1a887cca4c9dd2ac9fcb07405bbeda51b861b18bbf7492c4b",
        "hash_type": "type",
        "args": "0x95cd8c8413786f88cba646b0004737daeb7ffe2d"
      },
      "type": null
    },
    {
      "capacity": "0xe1614b916",
      "lock": {
        "code_hash": "0x9bd7e06f3ecf4be0f2fcd2188b23f1b9fcc88e5d4b65a8637b17723bbda3cce8",
        "hash_type": "type",
        "args": "0x95cd8c8413786f88cba646b0004737daeb7ffe2d"
      },
      "type": null
    }
  ],
  "outputs_data": [
    "0x",
    "0x"
  ],
  "header_deps": [],
  "witnesses": [
    "0x10000000100000001000000010000000"
  ]
}

Context transaction:

[
  {
    "version": "0x0",
    "cell_deps": [
      {
        "out_point": {
          "tx_hash": "0xf8de3bb47d055cdf460d93a2a6e1b05f7432f9777c8c474abf4eec1d4aee5d37",
          "index": "0x0"
        },
        "dep_type": "dep_group"
      }
    ],
    "inputs": [
      {
        "previous_output": {
          "tx_hash": "0x21d8986319846529f4e8ea5b612464c459e6f080c7af03e337ece1cef6d36d99",
          "index": "0x1"
        },
        "since": "0x0"
      }
    ],
    "outputs": [
      {
        "capacity": "0xf81ab5a00",
        "lock": {
          "code_hash": "0x9bd7e06f3ecf4be0f2fcd2188b23f1b9fcc88e5d4b65a8637b17723bbda3cce8",
          "hash_type": "type",
          "args": "0x95cd8c8413786f88cba646b0004737daeb7ffe2d"
        },
        "type": null
      },
      {
        "capacity": "0x4d0c5d488025",
        "lock": {
          "code_hash": "0x9bd7e06f3ecf4be0f2fcd2188b23f1b9fcc88e5d4b65a8637b17723bbda3cce8",
          "hash_type": "type",
          "args": "0x7bafd91d0ecdae1344b50b0bfd6a74a39e342532"
        },
        "type": null
      }
    ],
    "outputs_data": [
      "0x",
      "0x"
    ],
    "header_deps": [],
    "hash": "0x02510e8042bb4d53b10256dcb3cd1939a3c86239b497f0e7334cf78dc513f9ad",
    "witnesses": [
      "0x5500000010000000550000005500000041000000163149023968059ec6ac5fb6044526cc9a1cab2720b23ab806c3c040b1bcc1543767981b57572425beb707ec5b01321266ea1702dc7ee3b1cc0326589014d76101"
    ]
  }
]

Reproduction:

It seems that sUDT is only available in Neuron for JavaScript applications, so we need to test LedgerJS in Neuron:

  1. Clone this branch of Neuron.
  2. Configure the development environment and start Neuron according to the Neuron Quick Start instructions.
  3. Neuron -> Settings -> Network -> add testnet or devnet.
  4. Neuron -> Settings -> Wallets -> import hardware wallet.
  5. Open Chrome and type chrome://inspect/#devices in the address bar, click the configure button and type localhost:5858, then click Open dedicated DevTools for Node
  6. Neuron -> Sidebar -> Experimental -> Assets Accounts -> click plus icon -> create a sudt account or CKB account.
  7. If all goes well, you can debug Neuron in chrome. I’ve set a breakpoint in Neuron's signing function, so you can check if the data involved in signing is correct or not in chrome.
mikereinhart commented 3 years ago

Thanks a lot, @yuche! To briefly touch on all three:

yuche commented 3 years ago

Thanks a lot, @yuche! To briefly touch on all three:

  • Change Path during Regular Transaction: Can you confirm you're using obsidiansystems/ledgerjs@fc2c190? That git hash includes a fix to v0.1.0 of LedgerJS which touches on change paths.
  • DAO Deposit: I think this is due to an issue with dao deposit which we found in v0.4.0 of the Ledger app. We recently fixed it, I'll share an RC with you so you can confirm this is the case.
  • sUDT: sUDTs are not yet supported in the Ledger app and/or LedgerJS.

nano-s-0.4.1-rc1.tar.gz

mikereinhart commented 3 years ago

Sorry to hear that! We'll take a look at both of these issues and let you know what we find

mikereinhart commented 3 years ago

@yuche one more thing! What version of CKB-CLI are you using (in Neuron)? Thanks

yuche commented 3 years ago

@yuche one more thing! What version of CKB-CLI are you using (in Neuron)? Thanks

In Neuron we don't use CKB-CLI, but we have packaged the CKB instance(v0.34.1) itself into Neuron. The two problems mentioned above do not require Neuron to reproduce, they can be reproduced by running Node.js script from GitHub gist. Only sUDT needs Neuron to be reproduced.

change path: https://gist.github.com/yuche/b609442c55944110329a2387963d0df6 dao deposit: https://gist.github.com/yuche/51d25dda02b9495f94a981c965d1cd5f

katat commented 3 years ago

Hi @mikereinhart

Thanks for the help in getting these issues resolved.

For the sUDT issue, my assumption was that the ledgerjs can sign the transactions in a generic way no matter what smart contracts the transaction involves.

Could you please explain a bit technically why it is not supported in the current implementation of ledgerjs although it supports the DAO transaction, which seems to me has a similar transaction structure of sUDT (including additional cellDep and value in the data field)?

Also, would it be possible to have ledgerjs to support signing in a generic way, so that we don't need changes on ledgerjs for the other contract protocols in the future?

mikereinhart commented 3 years ago

The signing code is generic, but we need to be able to describe the transaction adequately to the user on the ledger's screen to allow the transaction. It believe it would be possible for us to come up with a solution that attempts to capture current and future contract protocols as best we can, but as you're aware that hasn't been within the scope of our efforts yet.

katat commented 3 years ago

If the sUDT restriction is due to the feature of describing transaction adequately on device, I think we can bypass this display feature for the transactions other than the ones of making standard transfers. Similar to how the ETH app handles the transactions for arbitrary contracts.

yuche commented 3 years ago

@mikereinhart Hi Mike. Thank you so much for your help in the past time. I noticed you mentioned that the bug fixes for LedgerJS will start this week, I was wondering if there is a timeline for fixing the change path and DAO deposit issues?

And now that Neuron's Ledger interactions are almost complete, we still need to resolve the UDT transaction issue. I'm fully aware that this is no easy task, but it would be helpful for Neuron's release and iteration planning to know the approximate time frame for the problem to be resolved.

mikereinhart commented 3 years ago

@yuche expect the change path and DAO deposit issues to be fixed before we add a generic solution for UDTs. While it is tough to say exactly when each will be delivered, we're working on change path/DAO deposit right now. Ultimately everything should be resolve before the end of next week!

Ericson2314 commented 3 years ago

Hi @yuche I've been trying to reproduce your issues and have been running into trouble. You can see so far what I have here: https://github.com/obsidiansystems/hw-app-ckb/pull/1 (starting with the change address issue.)

First of all, I pull out our ledger app from https://github.com/obsidiansystems/ledger-app-nervos/ and the nervos package to give it its own proper home in https://github.com/obsidiansystems/hw-app-ckb. I see you had uploaded it at https://www.npmjs.com/package/hw-app-ckb for easier consumption; hopefully having it in its own repo without the vendored blockchain-agnostic ledger deps will help you consume it in between releases.

Secondly, I got our example app (now at https://github.com/obsidiansystems/hw-app-ckb/blob/develop/examples/ckb-sdk-js-example/packages/main/bin/index.cjs) properly packaged, having it's own package.json to pull in the sdk, etc., and normalizing the require(..)s not to use any relative paths to temporary build products.

Now, on the PR, I added your first gist as another package.json bin entry, but am hitting issues with various deprecated mechanisms in the CKB node, etc. Perhaps I should might also confirm I'm using the same CKB node version as you, but I think I should also get our example app matching your gists in using the lumos indexer, etc.

I'll keep on poking at this, and will post back if I get it figured out, but any guidance you have locking down versions, avoiding deprecated old ways of doing things, etc., would be much appreciated. Perhaps ideally, going forward, we both can get the example package there in good enough shape that you could make PRs rather than gists for any future bugs, and I'd be able to reproduce right out of the gate. Hope all this can ease the process for both of us :).

yuche commented 3 years ago

Now, on the PR, I added your first gist as another package.json bin entry, but am hitting issues with various deprecated mechanisms in the CKB node, etc. Perhaps I should might also confirm I'm using the same CKB node version as you, but I think I should also get our example app matching your gists in using the lumos indexer, etc.

Hi, @Ericson2314.

I submitted two gist to a repository: https://github.com/yuche/ledger-issues. I've locked down all the dependent versions in package.json, which should reproduce successfully after running yarn.

Note that in the change-path.js file you need to change the code to suit your situation as per comments, dao.js should run without modification. And you can use npm link to link your local hw-app-ckb to the node_modules of the reproduce repo.

Ericson2314 commented 3 years ago

OK! I'm relieved to report I've finally gotten both issues reproduced. I had to make sure startSync for Lumos was called, otherwise no unspentCells were found. Then, when I realized my devnet was set up non-optimally, I had trouble getting Lumos to sync with the new one (deleting $TMP/lumos_db wasn't sufficient?), so I got the old deprecated non-Lumos way to work.

With that done, I could get debug messages from the ledger to get some more interesting errors:

Change:

Can't handle self-transactions with multiple non-change destination addresse

The dao one seems like a fairly clear-cut case of a dao cell being misidentified as non-dao cell.

Dao

Data found in non-dao cell

The change case is a bit more interesting. We need to different between change and non-change outputs to give more human-useful messages for the amount transferred, and the ledger app is complaining we gave it some sort of pathological case it refuses to handle.

I see two possibilities:

The transaction and annotations do not match

@yuche you earlier said

// error
await lckb.signTransaction("44'/309'/0'/0/0", formatted, [formatted.witnesses[0]], formattedCtxd, "44'/309'/0'/1/0")
await lckb.signTransaction("44'/309'/0'/0/1", formatted, [formatted.witnesses[1]], formattedCtxd, "44'/309'/0'/1/0")
// works
await lckb.signTransaction("44'/309'/0'/0/0", formatted, [formatted.witnesses[0]], formattedCtxd, "44'/309'/0'/0/0")
await lckb.signTransaction("44'/309'/0'/0/1", formatted, [formatted.witnesses[1]], formattedCtxd, "44'/309'/0'/0/0")

One thing that perhaps isn't clear is that the changePath parameter just effects the "annotation" (additional info separate from the raw transaction) being sent to the ledger, and not the raw transaction itself. In other words, it's important that the caller makes sure that formatted indeed has an output matching that bip-32 path. If that isn't that case, I wouldn't be surprise if an obscure error like this would arise.

Ledger app (not ledger JS) error

The other explanation that seems likely to me is that the java script / host side is doing everything right, and the ledger code to identify "self transactions" is a bit off. Indeed the condition surrounding this error message does make me a bit suspicious.

https://github.com/obsidiansystems/ledger-app-nervos/blob/1f3810581065a51c00808519e23ec6e8594d279c/src/apdu_sign.c#L580-L583

the English message sounds to me like && but the code says ||.

My hunch is that the second explanation is more likely, but it would be good to double check the first as it will be easier to fix.

I will confer with others a bit to get to the heart of the self-transaction code paths tomorrow, and perhaps try to make a speculos (ledger emulator) unit test of our app with the posted original transaction and context transaction.

mikereinhart commented 3 years ago

Hi @yuche and @katat - we have custom script support done and fixes for the change path and DAO bugs. There's a little bit more clean up to do, but we'll have a new hex file to you soon for you to confirm for yourself! Thanks

yuche commented 3 years ago

I will confer with others a bit to get to the heart of the self-transaction code paths tomorrow, and perhaps try to make a speculos (ledger emulator) unit test of our app with the posted original transaction and context transaction.

@Ericson2314 I think unit testing is really important. Without unit testing, there is a risk of fixing one problem but causing another. I'm glad that the Ledger App has added more unit tests, and if hw-app-ckb needs to write more unit tests, I'd be happy to help.

Hi @yuche and @katat - we have custom script support done and fixes for the change path and DAO bugs. There's a little bit more clean up to do, but we'll have a new hex file to you soon for you to confirm for yourself! Thanks

So glad to hear that, you guys are awesome! Please let us know when the new version is released.

mikereinhart commented 3 years ago

While this was mentioned in other channels, I wanted to note on this thread that any outstanding issues should all be address in the latest set of releases. They are:

yuche commented 3 years ago

While this was mentioned in other channels, I wanted to note on this thread that any outstanding issues should all be address in the latest set of releases. They are:

When Allow contract data is turned on, the message contract not present sometimes appears when signing non-sUDT transactions, and sometimes it does not. Is there an exact pattern that shows this message? Also, will turning on the Allow contract data option have any effect on non-sUDT signatures?

By default, Allow contract data is turned off, and an error is reported when signing a sUDT transaction:

Ledger device: Condition of use not satisfied (denied by the user?) (0x6985)

Is it possible that this error could be changed to suggest turning on the Allow contract data option?

mikereinhart commented 3 years ago

@yuche can you provide an example of an sUDT transaction where the device reports contract data is not present?

And now, allowing contract data will not impact the signature on a non-sUDT transaction.

mikereinhart commented 3 years ago

@yuche just following up to see if you had examples of an sUDT transaction where the device reported contract data was not present, otherwise I'll assume this report was erroneous. Thanks!

yuche commented 3 years ago

@mikereinhart Sorry for the late response. The issue we are having is not that sUDT transactions report contract data was not present, but that non-sUDT transactions are sometimes reported and sometimes not. We have not yet found an exact pattern that is reproducible, but this seems to have no impact to signing transaction.

mikereinhart commented 3 years ago

Ok, so to clarify this a bit: when doing a normal transaction (I.e. wallet transfer ...) and you have turned on Allow contract data in the app's Configuration, this normal, non-sUDT transaction sometimes reports that contract data is Present and sometimes it reports that it is not. Did I get that right?

yuche commented 3 years ago

Ok, so to clarify this a bit: when doing a normal transaction (I.e. wallet transfer ...) and you have turned on Allow contract data in the app's Configuration, this normal, non-sUDT transaction sometimes reports that contract data is Present and sometimes it reports that it is not. Did I get that right?

Yes, that's exactly right.

mikereinhart commented 3 years ago

Can this issue be closed?

katat commented 3 years ago

Sure, thanks @mikereinhart I am closing this now.

mikereinhart commented 3 years ago

No problem! I'm happy we were able to address everything :)