Zondax / ledger-icp

Apache License 2.0
16 stars 9 forks source link

Unexpected data type #157

Closed infu closed 2 years ago

infu commented 2 years ago

I am getting this error when trying to use it the way nns dapp uses it.

Error: A ledger error happened during signature:
Code: 27012
Message: "Data is invalid : Unexpected data type"

Does this support everything on IC or it is limited to Ledger and Neuron calls?

:link: zboto Link

ocluf commented 2 years ago

Hey Infu,

Could you provide a code example of what you are doing in your dapp?

infu commented 2 years ago

Hello, I set this repo https://github.com/infu/signtest It starts from the official DFX boilerplate and with minimal changes attempts to hardware sign a request when you click the button. It successfully prompts my browser to select ledger device, then this error shows:

ledger.js:68 Uncaught (in promise) Error: A ledger error happened during signature:
Code: 27012
Message: "Data is invalid : Unexpected data type"

    at LedgerIdentity.sign (ledger.js:68:1)
    at async LedgerIdentity.transformRequest (ledger.js:79:1)
    at async HttpAgent.call (index.js:172:1)
    at async caller (actor.js:174:43)
    at async HTMLFormElement.<anonymous> (index.js:17:1)
infu commented 2 years ago

Because NNS dapp uses its own similar Identity, I took it and tried again, but the same thing happens. Here is the second repository https://github.com/infu/signtest2 It uses https://github.com/dfinity/nns-dapp/blob/977a114ed197511c78044d885981494935f22085/frontend/ts/src/ledger/identity.ts instead of @dfinity/identity-ledgerhq

infu commented 2 years ago
Code: 27012
Message: "Data is invalid : Unexpected data type"

    at nns_identity.ts:168:1
    at async LedgerIdentity._executeWithApp (nns_identity.ts:205:1)
    at async LedgerIdentity.sign (nns_identity.ts:162:1)
    at async LedgerIdentity.transformRequest (nns_identity.ts:186:1)
    at async HttpAgent.call (index.js:172:1)
    at async caller (actor.js:174:43)
    at async HTMLFormElement.<anonymous> (index.js:17:1)
(anonymous) @ nns_identity.ts:168
infu commented 2 years ago

Expected behavior: Internet Computer Ledger app should be able to sign arbitrary requests to any canisters with any payload while providing the same interface. NNS is vital, but a small part of the IC. If the Ledger app can only sign requests towards NNS, it would be like having the Ethereum app, only signing native ETH transactions, while unable to sign a smart contract transaction. If that is not the case, please provide an example, currently, there are none in the whole Github.

AleDema commented 2 years ago

This also happens when merging neurons in the NNS frontend

jleni commented 2 years ago

Could you provide the blob that is sent to the device? We could then add that as an integration test as in here:

https://github.com/Zondax/ledger-icp/blob/fea25e572a8e03c1531ff83156dbf693da56969d/tests_zemu/tests/phase2.test.ts#L406-L407

The current app only supports protobuf based data.. My hunch is that you are sending candid-based payloads, which are logically rejected by the current version.

These are planned to be supported in the near future

AleDema commented 2 years ago

Nothing is shown on the ledger, when I confirm the merge operation I get a Chrome alert stating: Code: 27012 Message: "Data is invalid : Unexpected data type"

ielashi commented 2 years ago

I think this is a UI problem and not a problem with the Ledger app. The "merge neuron" functionality isn't yet implemented in the Ledger app unfortunately, but it's on the roadmap. I'll be chatting with the NNS UI team for them to inform users of this whenever they try to merge neurons.

lmuntaner commented 2 years ago

Expected behavior: Internet Computer Ledger app should be able to sign arbitrary requests to any canisters with any payload while providing the same interface. NNS is vital, but a small part of the IC. If the Ledger app can only sign requests towards NNS, it would be like having the Ethereum app, only signing native ETH transactions, while unable to sign a smart contract transaction. If that is not the case, please provide an example, currently, there are none in the whole Github.

Hi,

The supported transactions by the Ledger app is at the moment limited to a subset of all the transactions. We are working at the moment on a migration to a new payload parser and also adding four new transaction types asked by the community which are:

When you get the error "Data is invalid : Unexpected data type", it usually means that the transaction is not supported.

In the UI, we try to provide a good UX by disabling the functionality that is not yet supported.

I hope this helped understand the problem.

Let me know if you have any other questions!

infu commented 2 years ago

"We are working at the moment on a migration to a new payload parser" How are we gonna find out you added the functionality. Isn't the issue supposed to be open until it's added? Or do we create another issue.

jleni commented 2 years ago

I understand your point, however, supporting "everything" is too broad:

Does this support everything on IC or it is limited to Ledger and Neuron calls?

Would it be possible for you to create another issue indicating the specific tx types that you need support for?

Maybe if you can provide the specific blob/tx that you were testing, we can include it in the roadmap?

skilesare commented 2 years ago

Why is it too broad? Candid is supposed to be extensible. We just want candid transactions supported.