search

vacuumlabs / cardano-hw-cli

Cardano CLI tool for hardware wallets
71 stars 24 forks source link

Add cardano-hw-interop-lib #110

Closed davidmisiak closed 2 years ago

davidmisiak commented 2 years ago

All tx parsing and tx types are replaced by interop-lib. Apart from that, I tried to make ledgerCryptoProvider and TrezorCryptoProvider a bit more consistent with each other.

Some changes in interop-lib were necessary, see https://github.com/vacuumlabs/cardano-hw-interop-lib/pull/2. This is the branch referenced in package.json (you need to clone interop-lib, checkout the prepare-for-integration branch and run yarn install && yarn build && yarn pack).

I don't have a Ledger device, so I couldn't test it (manually nor using integration tests).

davidmisiak commented 2 years ago

Please update mocha to have --dry-run option (see ledgerjs).

Done.

I tried yarn test-integration-ledger and it all passed except "Should sign tx ordinary_ByronInputAndOutput" which fails on "DeviceStatusError: Action rejected by Ledger's security policy".

The reason might be that ledger now has single account model, but the signing files hwSigningFiles: [signingFiles.payment0, signingFiles.byron10] do not seem to violate it (both use account 0). I've not investigated it further.

I don't see why this fails, and I'm afraid I can't investigate this further without a Ledger device (and knowledge). Shall I ask Marton to take a look?

janmazak commented 2 years ago

I tried yarn test-integration-ledger and it all passed except "Should sign tx ordinary_ByronInputAndOutput" which fails on "DeviceStatusError: Action rejected by Ledger's security policy". The reason might be that ledger now has single account model, but the signing files hwSigningFiles: [signingFiles.payment0, signingFiles.byron10] do not seem to violate it (both use account 0). I've not investigated it further.

I don't see why this fails, and I'm afraid I can't investigate this further without a Ledger device (and knowledge). Shall I ask Marton to take a look?

I've fixed it, the problem is within the Ledger app and unrelated to hw-cli.

refi93 commented 2 years ago

@davidmisiak I was trying building the interop-lib because @gitmachtl asked me and when running yarn install I got the following error

➜  cardano-hw-interop-lib git:(prepare-for-integration) yarn install
yarn install v1.22.10
[1/4] Resolving packages...
[2/4] Fetching packages...
error https://gitpkg.now.sh/hildjj/node-cbor/packages/cbor?main: Integrity check failed for "cbor" (computed integrity doesn't match our records, got "sha512-62QB1mXm4yKIf0K1PXqb9HOai2w7NBuaet/KQDsizKv18YaVdEgYshsLHJmN0z1t4NoXF2WgJEZtaPLnzhjc0g== sha1-oYLtW1yryhehiGbNxDExW8DB7X4=")
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.

I think this is due to the node-cbor package being obtained from a branch (main) that meanwhile got new commits and the package built from it now has a different checksum. I guess we can set the node-cbor to version 8.1.0 already in package.json as I see that @KubqoA 's fix has already been merged there and released to npm: https://www.npmjs.com/package/cbor

KubqoA commented 2 years ago

@davidmisiak I was trying building the interop-lib because @gitmachtl asked me and when running yarn install I got the following error

➜  cardano-hw-interop-lib git:(prepare-for-integration) yarn install
yarn install v1.22.10
[1/4] Resolving packages...
[2/4] Fetching packages...
error https://gitpkg.now.sh/hildjj/node-cbor/packages/cbor?main: Integrity check failed for "cbor" (computed integrity doesn't match our records, got "sha512-62QB1mXm4yKIf0K1PXqb9HOai2w7NBuaet/KQDsizKv18YaVdEgYshsLHJmN0z1t4NoXF2WgJEZtaPLnzhjc0g== sha1-oYLtW1yryhehiGbNxDExW8DB7X4=")
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.

I think this is due to the node-cbor package being obtained from a branch (main) that meanwhile got new commits and the package built from it now has a different checksum. I guess we can set the node-cbor to version 8.1.0 already in package.json as I see that @KubqoA 's fix has already been merged there and released to npm: https://www.npmjs.com/package/cbor

@refi93 I updated the library right now to use the node-cbor version 8.1.0, so this should be resolved.

gitmachtl commented 2 years ago

i have checked the code, but there is currently no implementation for the transformTxBody or transformRawTx exposed as a new cli command available right?

davidmisiak commented 2 years ago

i have checked the code, but there is currently no implementation for the transformTxBody or transformRawTx exposed as a new cli command available right?

That's correct, I'm going to add it soon.

gabrielKerekes commented 2 years ago

@gitmachtl thanks for reaching out (many times already) and testing the code. We were wondering if you, as a user of HW CLI, could help us validate the user flow we plan to implement regarding the interoperability library and HW CLI.

The goal of the interoperability library is to provide these use cases to its users:

However, we need to (or we think we need to) support multiple input formats for the transactions:

  1. raw transaction CBOR - this is what the cardano-cli transaction build-raw and perhaps also build commands return
  2. signed, CDDL compliant transaction
  3. transaction body CBOR (this is not “needed” per se, but it might be useful for interoperability lib users) This means that there will be 6 exposed calls from the interoperability library (exact naming is TBD): validate-raw-tx, validate-tx, validate-tx-body and transform-raw-tx, transform-tx, transform-tx-body.

When it comes to the HW CLI, it will expose four new commands: transaction validate (will validate a signed, CDDL compliant transaction), transaction validate-raw, transaction transform and transaction transform-raw. Each command will take a tx file as input. The transaction transform call will only work if the transaction contains no witnesses since “transforming” the transaction will invalidate existing witnesses.

Does this make sense to you? Is there something we’re missing? Would this cover your use cases? We appreciate your help 🙏

gitmachtl commented 2 years ago

hey @gabrielKerekes, first - thanks to you guys for your hard work. the community really appreciate it!

the most important thing i would say is the validation and the transformation of the raw transaction cbor (output from cardano-cli transaction build-raw). the buildcommand generates the same kind of output, it just makes things a bit easier for the fee calculation etc. the thing is that each change in the transformation will automatically also change the tx-id. this can be calculated from the unsigned tx raw and also from the signed one. so i am not sure if we need the option to transform an already signed transaction. if the order is not right, cardano-hw-cli will fail at the initial call like a witness signing or any other signing. so i am not sure when the scenario to transform a signed transaction comes into place. but if its already implemented in your lib, why not. collecting signing keys is done via providing multiple witnesses and combine them at the end. the transformation of the unsigned raw tx should take place before the witness signing round(s) so the tx-id stays the same. The transaction body cbor transformation/validation is nice yep. i am happy to do some testing on this, i have my "non working" transactions ready to be resolved :-)

Does this make sense to you? Is there something we’re missing? Would this cover your use cases? We appreciate your help 🙏

yes, this totally covers it. we should be good with that.

question, the input is a tx file, there will be an output tx file too right? or is it an inplace transformation on the same tx file?

HW CLI will not autocorrect tx files on the fly during the signing process without any notification right? a wrong tx file will still cause HW CLI to exit with an error?

janmazak commented 2 years ago

@gitmachtl We believe that the option to transform an "already signed transaction" is relevant for multisig and Plutus. The "already signed" tx will probably be a CDDL-compliant tx with script witnesses (native or Plutus) but lacking vkey witnesses.

It is unlikely that such yet-to-be-signed txs will be distributed in the raw internal cardano-cli format --- dapps (Plutus off-chain tools) and multisig wallets are much more likely to use the CDDL format (with possibly some elements missing, e.g. with an empty list of vkey witnesses). IOHK agrees that this is the most reasonable way for a universal format, and cardano-cli might move in that direction, too (it is being investigated, no definite conclusions yet).

gitmachtl commented 2 years ago

@janmazak ah ok, thx. clears things up, haven't thought about that.

davidmisiak commented 2 years ago

question, the input is a tx file, there will be an output tx file too right? or is it an inplace transformation on the same tx file?

Yes, the transformation call will have two arguments - input file and output file.

HW CLI will not autocorrect tx files on the fly during the signing process without any notification right? a wrong tx file will still cause HW CLI to exit with an error?

Yes, I think refusing to sign/witness a noncanonical transaction is the most clear option, so that the user has to manually call transform-raw instead of us modifying their transaction (even with a notice).

davidmisiak commented 2 years ago

I've just added the validation and transformation calls. I will test it more and submit some transactions on Monday.

@gitmachtl feel free to try it out. We will be thankful for any feedback!

gitmachtl commented 2 years ago

I've just added the validation and transformation calls. I will test it more and submit some transactions on Monday.

@gitmachtl feel free to try it out. We will be thankful for any feedback!

thx @davidmisiak , will try to do some testing on the weekend. 😄

gitmachtl commented 2 years ago

@davidmisiak what am i missing here?

i built/install/pack the cardano-hw-interop-lib from the branch "prepare-for-integration". in the cardano-hw-cli package.json i changed the path for the cardano-hw-interop-lib to the packed tgz file.

when i try to run yarn build-linux-tar i get errors, like that:

src/commandExecutor.ts:1:29 - error TS2307: Cannot find module 'cardano-hw-interop-lib' or its corresponding type declarations.

1 import * as InteropLib from 'cardano-hw-interop-lib'
                              ~~~~~~~~~~~~~~~~~~~~~~~~

src/crypto-providers/ledgerCryptoProvider.ts:1:26 - error TS2307: Cannot find module 'cardano-hw-interop-lib' or its corresponding type declarations.

1 import * as TxTypes from 'cardano-hw-interop-lib'
                           ~~~~~~~~~~~~~~~~~~~~~~~~

when i add an

"include": ["../cardano-hw-interop-lib/src/types.ts"]

in the tsconfig.json and run the yarn build-linux-tar command again, it compiles and links without an error.

but when i try to execute the hw-cli like ./cardano-hw-cli version i get errors like:

import { CommandType, parse } from './command-parser/commandParser'
^^^^^^

SyntaxError: Cannot use import statement outside a module
    at wrapSafe (internal/modules/cjs/loader.js:1070:16)
    at Module._compile (internal/modules/cjs/loader.js:1120:27)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1176:10)
    at Module.load (internal/modules/cjs/loader.js:1000:32)
    at Function.Module._load (internal/modules/cjs/loader.js:899:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:74:12)
    at internal/main/run_main_module.js:18:47

can you give me a hint?

davidmisiak commented 2 years ago

@gitmachtl Your building steps seem to be perfectly correct. Unfortunately, I can't reproduce this issue. Some ideas:

  1. Make sure you are using Node 12.19 when building interop-lib as well as hw-cli.
  2. Yarn uses .tgz package caching very aggressively, so try to rename the .tgz file to a name you've never used before (that's the reason why I have the obscure 28 in package.json: "cardano-hw-interop-lib": "../cardano-hw-interop-lib/cardano-hw-interop-lib-v1.0.0-28.tgz").
  3. Maybe deleting node_modules and purging yarn cache could help?

If none of these tips resolve your issue, we can investigate this further. (Does anybody else encounter this? Eg. @janmazak?) Until then, I recommend trying yarn build-js and running hw-cli as node dist/index.js, maybe at least that could work.

gitmachtl commented 2 years ago

@davidmisiak thx, yes node was set correct. i reinstalled tsc, and that did the trick. don't ask me why, but it worked now 😄

gitmachtl commented 2 years ago

@davidmisiak first test:

ISSUE: Why does the HW CLI need a connected ledger/trezor device to do the validation/transformation? This function should be independent of a connected HW device imo, because otherwise we can't use it in scenarios that just prepares transactions to be signed on other systems later for example. There is no need to check for a connected device for these function calls.

WITHOUT a ledger connected:

$ ./cardano-hw-cli transaction validate-raw --tx-body-file test.txbody
Error: Error occured while trying to find hw transport, make sure Ledger or Trezor is connected to you computer

WITH a ledger connected and the cardano-app open:

$ ./cardano-hw-cli transaction validate-raw --tx-body-file test.txbody
The transaction contains following fixable errors:
  CBOR is not canonical (transaction)

Transformation test:

$  ./cardano-hw-cli transaction transform-raw --tx-body-file test.txbody --out-file test-out.txbody
The transaction contains following fixable errors:
  CBOR is not canonical (transaction)
Fixed transaction will be written to the output file.

Retesting the generated txbody file:

./cardano-hw-cli transaction validate-raw --tx-body-file test-out.txbody
The transaction CBOR is valid and canonical.

I saw that the exit code is always 0, Would it be ok to parse for the keyword "error" in the last line of the output?

Will have to do some transaction tests, but the addresses with the tokens i made for testing were living on the alonzo-purple chain, and that chain died a few days ago... so i have to make new ones.

gitmachtl commented 2 years ago

ok, found some other issues.

first, i minted different assets on a ledger address like: image

that worked like it should.

than i tried to mint an addition asset with a shorter namelength but a higher starthexcode. cardano-cli is doing a wrong sorting here. so i force to get a TxBody that is not in canonical order.

without the implemented transform-raw step i get the following output: image

that looks promising, it detected the wrong order.

ok, but when i implement the transform-raw before the witness signing step it looks like this: image

the corrected output was written out, but when it comes to the witness signing, cardano-hw-cli aborts with the following error: Command failed: transaction witness Error: /tmp/ledger-red.payment.txbody: TextEnvelope decode error: DecoderErrorDeserialiseFailure "Shelley TxBody" (DeserialiseFailure 219 "expected list start")

the TxBodyFile before the correction was:

{
    "type": "TxBodyMary",
    "description": "",
    "cborHex": "83a500818258205ecbc2e3779c88ef83193e0788bd0ca3ee8e839695736ca2bd1a0d5d17c74d660001818258390074976c54afaf444f7cd499bd8519aac6592b13b22b9d5817f0da5c5203d205532089ad2f7816892e2ef42849b7b52788e41b3fd43a6e01cf821b000000e8d4c51c9ba1581c2e2f143b3ccbe339145183dc2a799a469e92ab56e0d5b0bd04f54f15a34018644611223322110018c84568616c6c6f19012c021a0002cc9d031a029b6d6f09a1581c2e2f143b3ccbe339145183dc2a799a469e92ab56e0d5b0bd04f54f15a14568616c6c6f19012c9f8200581c344e624c22c7ee61264c4307591913c63adf36495b1fec6fdc5670cefff6"
}

after the correction:

{
    "type": "TxBodyMary",
    "description": "",
    "cborHex": "83a500818258205ecbc2e3779c88ef83193e0788bd0ca3ee8e839695736ca2bd1a0d5d17c74d660001818258390074976c54afaf444f7cd499bd8519aac6592b13b22b9d5817f0da5c5203d205532089ad2f7816892e2ef42849b7b52788e41b3fd43a6e01cf821b000000e8d4c51c9ba1581c2e2f143b3ccbe339145183dc2a799a469e92ab56e0d5b0bd04f54f15a34018644568616c6c6f19012c4611223322110018c8021a0002cc9d031a029b6d6f09a1581c2e2f143b3ccbe339145183dc2a799a469e92ab56e0d5b0bd04f54f15a14568616c6c6f19012c818200581c344e624c22c7ee61264c4307591913c63adf36495b1fec6fdc5670cef6"
}

that was for minting assets on a ledger address directly. i will now try to just send assets around that are already on a ledger address...

gitmachtl commented 2 years ago

ok, a normal transaction from a ledger address with transform-raw involded looks good:

image

next test is to mint with the policy on the ledger ...

gitmachtl commented 2 years ago

this test also failed, as i wrote above this is with the policy on the hw-ledger. the witness signing on the ledger is working, going thru all the steps on the GUI. the failing point is cardano-cli: image

So far it looks like cardano-cli is having a problem with the transformed txbody when it comes to the witness generation like

$ cardano-cli transaction witness --tx-body-file ${txBodyFile} --signing-key-file ${fromAddr}.skey ${magicparam} --out-file ${txWitnessPaymentFile}

for a payment signing key witness, or

$ cardano-cli transaction witness --tx-body-file ${txBodyFile} --signing-key-file ${policyName}.policy.skey ${magicparam} --out-file ${txWitnessPolicyFile}

for generating the policy witness.

The used TxBodyFiles in this test were the following...

BEFORE correction:

{
    "type": "TxBodyMary",
    "description": "",
    "cborHex": "83a5008282582019d5fec5f1e7f487ed04ad91c7f9eed2bfeb14feef6f7a8c3b6a05af6de10314018258205f0635d1e02d18b9020b932d472b1cd5dea17c4c66259be443be2a8710838ed900018182581d6052e63f22c5107ed776b70f7b92248b02552fd08f3e747bc745099441821b0000000124f36937a2581c2e2f143b3ccbe339145183dc2a799a469e92ab56e0d5b0bd04f54f15a34018644611223322110018c84568616c6c6f19012c581c6b79ff1505414bd7ebba7a9b3242ba02810f0b91ea9e391c6e0b7409a143787878190190021a0002db0d031a029b773909a1581c6b79ff1505414bd7ebba7a9b3242ba02810f0b91ea9e391c6e0b7409a1437878781901909f8200581c8d87ba19c400938453e86abf8c2f1f9a94c1acf7f495978e70ab0068fff6"
}

AFTER correction:

{
    "type": "TxBodyMary",
    "description": "",
    "cborHex": "83a5008282582019d5fec5f1e7f487ed04ad91c7f9eed2bfeb14feef6f7a8c3b6a05af6de10314018258205f0635d1e02d18b9020b932d472b1cd5dea17c4c66259be443be2a8710838ed900018182581d6052e63f22c5107ed776b70f7b92248b02552fd08f3e747bc745099441821b0000000124f36937a2581c2e2f143b3ccbe339145183dc2a799a469e92ab56e0d5b0bd04f54f15a34018644568616c6c6f19012c4611223322110018c8581c6b79ff1505414bd7ebba7a9b3242ba02810f0b91ea9e391c6e0b7409a143787878190190021a0002db0d031a029b773909a1581c6b79ff1505414bd7ebba7a9b3242ba02810f0b91ea9e391c6e0b7409a143787878190190818200581c8d87ba19c400938453e86abf8c2f1f9a94c1acf7f495978e70ab0068f6"
}

note also the difference in the ending from fff6 to just f6.

gitmachtl commented 2 years ago

leaving the hw wallet completly out of the equation and just doing a normal transaction with a transform-raw to get the canonical order also results in the same failure with cardano-cli:

image

The used TxBodyFiles in this test were the following...

BEFORE correction:

{
    "type": "TxBodyMary",
    "description": "",
    "cborHex": "83a4008282582019d5fec5f1e7f487ed04ad91c7f9eed2bfeb14feef6f7a8c3b6a05af6de10314018258205f0635d1e02d18b9020b932d472b1cd5dea17c4c66259be443be2a8710838ed900018182581d6052e63f22c5107ed776b70f7b92248b02552fd08f3e747bc745099441821b0000000124f39563a1581c2e2f143b3ccbe339145183dc2a799a469e92ab56e0d5b0bd04f54f15a34018644611223322110018c84568616c6c6f19012c021a0002aee1031a029b79f39ffff6"
}

AFTER correction:

{
    "type": "TxBodyMary",
    "description": "",
    "cborHex": "83a4008282582019d5fec5f1e7f487ed04ad91c7f9eed2bfeb14feef6f7a8c3b6a05af6de10314018258205f0635d1e02d18b9020b932d472b1cd5dea17c4c66259be443be2a8710838ed900018182581d6052e63f22c5107ed776b70f7b92248b02552fd08f3e747bc745099441821b0000000124f39563a1581c2e2f143b3ccbe339145183dc2a799a469e92ab56e0d5b0bd04f54f15a34018644568616c6c6f19012c4611223322110018c8021a0002aee1031a029b79f380f6"
}

I retried it again with an already valid TxBodyFile and the error is the same:

image

And here are the TxBodyFiles before and after the transform-raw.

BEFORE correction:

{
    "type": "TxBodyMary",
    "description": "",
    "cborHex": "83a40081825820d1fb2cfb9d1c3b2352cfe0447390fa8811d36e004fd65cdf008e393cca032a7301018182581d6052e63f22c5107ed776b70f7b92248b02552fd08f3e747bc745099441821b0000000124dc4fdca1581c2e2f143b3ccbe339145183dc2a799a469e92ab56e0d5b0bd04f54f15a24018644611223322110018c8021a0002a045031a029b7b7a9ffff6"
}

AFTER correction:

{
    "type": "TxBodyMary",
    "description": "",
    "cborHex": "83a40081825820d1fb2cfb9d1c3b2352cfe0447390fa8811d36e004fd65cdf008e393cca032a7301018182581d6052e63f22c5107ed776b70f7b92248b02552fd08f3e747bc745099441821b0000000124dc4fdca1581c2e2f143b3ccbe339145183dc2a799a469e92ab56e0d5b0bd04f54f15a24018644611223322110018c8021a0002a045031a029b7b7a80f6"
}

there should be no difference in the TxBodyFiles for that, but there is again a problem with the array at the end of the cbor.

its changing from:

... 
     02                                # unsigned(2)
      1A 0002A045                       # unsigned(172101)
      03                                # unsigned(3)
      1A 029B7B7A                       # unsigned(43744122)
   9F                                   # array(*)
      FF                                # primitive(*)
   F6                                   # primitive(22)

to just:

...
      02                                # unsigned(2)
      1A 0002A045                       # unsigned(172101)
      03                                # unsigned(3)
      1A 029B7B7A                       # unsigned(43744122)
   80                                   # array(0)
   F6                                   # primitive(22)

@refi93, we had this issue in the past too... can't really remember what the outcome of the fix was back in the days.

davidmisiak commented 2 years ago

@gitmachtl thank you very much for testing!

All issues you found should be fixed:

  1. A connected device is no longer required for validation and transformation calls.
  2. Validation calls return different exit codes based on the validation result (see README.md for details).
  3. I managed to preserve the indefinite-length array format of the rawTx item, so cardano-cli should be able to parse it correctly. The fix is in interop-lib, so pulling and rebuilding it is necessary.

In case you discover any other problem (or you consider the provided fixes insufficient in some way), please let us know.

gitmachtl commented 2 years ago

@davidmisiak looks promising, i repeated the tests that i have done so far, now all passed. i combined a few in one asset minting transaction (see below). and it worked well. will continue some testing for the "basic" functions. stakekey reg/dereg, pool cold key gen, pool registrations, etc...

image

gitmachtl commented 2 years ago

ok, i have tested the following functions with real transactions on a ledger nano s without issues:

versions used:

gitmachtl commented 2 years ago

may i ask whats the current cardano-hw-cli release plan? will we get an intermediate release that includes the new transform functions within cardano-hw-cli but without the alonzo capabilities?

davidmisiak commented 2 years ago

may i ask whats the current cardano-hw-cli release plan? will we get an intermediate release that includes the new transform functions within cardano-hw-cli but without the alonzo capabilities?

@gitmachtl I've just created a pre-release. We do plan a proper release with these changes and without alonzo capabilities (probably in a week or two).

ok, i have tested the following functions with real transactions on a ledger nano s without issues:

Thanks for reporting!

gitmachtl commented 2 years ago

@davidmisiak

I've just created a pre-release. We do plan a proper release with these changes and without alonzo capabilities (probably in a week or two).

thx, cool. that should also trigger the release of cardano-app 3.0.0 via ledger live then...

gitmachtl commented 2 years ago

@davidmisiak i have done some testing also with trezor firmware (@38c5267). the creation of a hw policy is working, also the witness signing with the policy & minting. address generation too. but the generation of the pool cold key failed, is this still not included in trezor fw 2.4.4?

Do you have a chart somewhere what is included in the ledger and the trezor firmware? Or is this the only function that is currently not available in the trezor firmware?

Thx!

gabrielKerekes commented 2 years ago

but the generation of the pool cold key failed, is this still not included in trezor fw 2.4.4

Pool cold key support isn't available on Trezor, no. There is also no support for signing a transaction as pool operator. AFAIK there is currently no plan to add it. If there are many pool operators wanting to use Trezor then it perhaps would make sense to add it, but AFAIK we haven't received any such requests so far.

Do you have a chart somewhere what is included in the ledger and the trezor firmware?

There is currently no chart and I think this should be the only functionality missing in Trezor, but perhaps others can correct me if I am wrong.

gitmachtl commented 2 years ago

Pool cold key support isn't available on Trezor, no. There is also no support for signing a transaction as pool operator. AFAIK there is currently no plan to add it. If there are many pool operators wanting to use Trezor then it perhaps would make sense to add it, but AFAIK we haven't received any such requests so far.

thx, will do some preconditional checks in the spo scripts then.