search

celo-org / developer-tooling

🛠️ SDKs and CLI for interacting with Celo
Apache License 2.0
9 stars 4 forks source link

celocli crashes when unlocking funds with ledger ("Ledger device: UNKNOWN_ERROR (0x6b26)") #195

Open pputman-clabs opened 3 months ago

pputman-clabs commented 3 months ago

Package

@celo/celocli

Have you ensured that all of these are up to date?

What version of the package are you on?

patrick@C3X2C4YGJ0 Downloads % npm list -g 
/Users/patrick/.nvm/versions/node/v18.19.1/lib
 ├── @celo/celocli@4.2.0 
 ├── bip39@ 
 ├── corepack@0.22.0 
└── npm@10.2.4

What command or function is the bug in?

celocli releasecelo:locked-gold

Operating System

macOS (Apple Silicon)

Describe the bug

When unlocking funds, the OS on ledger seems to crash and restarts. celocli also crashes. See below:

patrick@C3X2C4YGJ0 Downloads % celocli releasegold:locked-gold --contract $CELO_RG_ADDRESS --action unlock  --useLedger --value 100578303998975999900
 ›   Warning: releasegold:locked-gold is not a celocli command.
Did you mean releasecelo:locked-gold? [y/n]: y
Retrieving derivation Paths [ 0 ]
Running Checks:
   ✔  0xFfb9F456d3eef70aB79C58ea9b038513139612c8 is a registered Account 
   ✔  0xFfb9F456d3eef70aB79C58ea9b038513139612c8 is not currently voting on a governance proposal 
   ✔  Account has at least 100.5783039989759999 non-voting Locked Gold over requirement 
All checks passed
maxFeePerGas and maxPriorityFeePerGas are not supported on Ledger yet. Automatically using gasPrice instead.
Sending Transaction: lockedGoldUnlock... failed: Ledger device: UNKNOWN_ERROR (0x6b26)
    TransportStatusError: Ledger device: UNKNOWN_ERROR (0x6b26)
arthurgousset commented 3 months ago

Just to be sure, could you try the command with releasecelo:locked-gold explicitly instead 

- % celocli releasegold:locked-gold --contract $CELO_RG_ADDRESS --action unlock  --useLedger --value 100578303998975999900
+ % celocli releasecelo:locked-gold --contract $CELO_RG_ADDRESS --action unlock  --useLedger --value 100578303998975999900

Probably not the cause the of the problem, but good to be extra sure.

pputman-clabs commented 3 months ago

Tried with releasecelo, got the same issue.

patrick@C3X2C4YGJ0 Downloads % celocli releasecelo:locked-gold --contract $CELO_RG_ADDRESS --action unlock  --useLedger --value 100578303998975999900
Retrieving derivation Paths [ 0 ]
Running Checks:
   ✔  0xFfb9F456d3eef70aB79C58ea9b038513139612c8 is a registered Account 
   ✔  0xFfb9F456d3eef70aB79C58ea9b038513139612c8 is not currently voting on a governance proposal 
   ✔  Account has at least 100.5783039989759999 non-voting Locked Gold over requirement 
All checks passed
maxFeePerGas and maxPriorityFeePerGas are not supported on Ledger yet. Automatically using gasPrice instead.
Sending Transaction: lockedGoldUnlock... failed: Ledger device: UNKNOWN_ERROR (0x6b26)
    TransportStatusError: Ledger device: UNKNOWN_ERROR (0x6b26)
arthurgousset commented 3 months ago

@nikosfrestisclabs pointed out this might be duplicate of

arthurgousset commented 3 months ago

@nicolasbrugneaux kindly agreed to own this issue. He'll start working on it on Mon, Mar 25.

aaronmboyd commented 1 month ago

Just chiming in also getting this error. Bit of a problem for validators, I am trying to change signers and can't use Ledger keys right now. Trying to revert to node 14 / celocli 2.5.1 or 3.0.0 to get it to work. Would love a fix for this!

Is there a confirmed workaround or do I have to derive my private keys and inject them directly to the cli to manage the validator?

arthurgousset commented 1 month ago

Just chiming in also getting this error. Bit of a problem for validators, I am trying to change signers and can't use Ledger keys right now. Trying to revert to node 14 / celocli 2.5.1 or 3.0.0 to get it to work. Would love a fix for this!

Is there a confirmed workaround or do I have to derive my private keys and inject them directly to the cli to manage the validator?

Thanks for chiming in @aaronmboyd 👋 @nicolasbrugneaux has been working on a fix, and has been our owner for all things Ledger. He's the best point of contact for this bug. I'll let him get back to you.

zviadm commented 1 month ago

@nicolasbrugneaux @arthurgousset just some notes that might be helpful: I am pretty sure these Ledger & Celo issues are related to changes that came with the Eth 2.0 (or London fork) changes in transaction format. Between @celo/connect v4 and v5, transactions changed from using the "gasPrice" stuff to using the new "maxGasFee..." stuff.

Unfortunately, corresponding changes never happened in the Celo Ledger app: https://github.com/celo-org/celo-ledger-spender-app . That stuff still expects old school transactions.

Also the error that it is throwing is I am pretty sure a misnomer. I am fairly certain the Celo-Ledger-Spender-App is jsut throwing this error: https://github.com/celo-org/celo-ledger-spender-app/blob/70d1c4936091557b6edb71d739499585768457a5/src/main.c#L2693 Because transaction parsing is just failing. Notice the error code: 0x6A80

And then Ledger/hw-app-eth helpfully relabels that error to something that is completely incorrect: https://github.com/LedgerHQ/ledger-live/blob/afffa7401380a223096107b6ca3c77c438194898/libs/ledgerjs/packages/hw-app-eth/src/Eth.ts#L52

And that is how you get "EthAppPleaseEnableContractData" errors. Even though I am pretty sure errors are way more fundamental, because celo-ledger-spender-app straight up can't decode new "gas price" type of transactions.

aaronmgdr commented 3 weeks ago

Hey @zviadm thanks for the report and sorry we didnt respond earlier.

Its true we started defaulting to maxFeePerGas rather than gasPrice, however we quickly added a fix where it defaults back to gasPrice for all transactions signed by ledger. I suggest trying @celo/contractkit@7.2.0

@nicolasbrugneaux has spent the last few months updating the software that connects ledger to contractkit. And we hired a company (bloo) that has expertise in ledger apps to create an update to celo-ledger-spender. which will support eip1559 and cip64 transactions. It should be available on ledger live in dev mode within a week or two. We believe this new ledger app and the updates the the js packages will solve the remaining issues.

zviadm commented 3 weeks ago

Hey @zviadm thanks for the report and sorry we didnt respond earlier.

Its true we started defaulting to maxFeePerGas rather than gasPrice, however we quickly added a fix where it defaults back to gasPrice for all transactions signed by ledger. I suggest trying @celo/contractkit@7.2.0

@nicolasbrugneaux has spent the last few months updating the software that connects ledger to contractkit. And we hired a company (bloo) that has expertise in ledger apps to create an update to celo-ledger-spender. which will support eip1559 and cip64 transactions. It should be available on ledger live in dev mode within a week or two. We believe this new ledger app and the updates the the js packages will solve the remaining issues.

Have you actually tested it out though? I don't think that change works anyways. While you do set the "gasPrice" and remove "maxGasFee" and "maxPriorityFeePerGas", I think transaction format is still different and it still fails on the Ledger. At least I can not get @celo/connect v5 to work with Ledger in any way. And considering celocli doesn't work either, I am guessing it isn't actually fixed.

There is also the new "type" parameter that transactions have right? Maybe that is what is making it not backwards compatible still.

EDIT: I tried to debug this further. I think the real issue is that the "feeCurrency" isn't set, so this still doesn't make the transaction a "celo-legacy" type. I tested this with celocli too. This doesn't work: celocli lockedgold:withdraw --useLedger ...params...

But this does work: celocli lockedgold:withdraw --useLedger --gasCurrency=0x765DE816845861e75A25fCA122bb6898B8B1282a ...params...

This can be worked around in the usage of SDK i think by forcefully setting "gasCurrency" even when you want to use CELO.

EDIT#2: Unfortunately I think feeCurrency can't be when you want to use CELO. it has to be null, so it automatically fails to pass this check: https://github.com/celo-org/developer-tooling/blob/4007db23d87b36624cd3a17fd429a4507d2e284b/packages/sdk/wallets/wallet-base/src/signing-utils.ts#L326

So you can use Ledger with other feeCurrencies, but i think without fixes somewhere in the SDK, it isn't possible to use it with CELO