Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature - Adding support to Flex device #22

Merged
merged 20 commits into from
Oct 23, 2024

Conversation

Z4karia
Copy link
Contributor

@Z4karia Z4karia commented Aug 22, 2024

Checklist

  • App update process has been followed
  • Target branch is develop
  • Application version has been bumped

@Z4karia
Copy link
Contributor Author

Z4karia commented Aug 22, 2024

Hello @agrojean-ledger, can you also merge this PR? It was supposed to be included in the PR#21 that you just merged. Thank you !

@Z4karia
Copy link
Contributor Author

Z4karia commented Aug 22, 2024

The failing test is due to the Ledger app database that does not have the flex flag declared for the celo app. A PR has been submitted: #247

@vforgeoux-ledger
Copy link
Member

Hey @Z4karia , can you please use the latest useCases for Flex/Stax ? https://developers.ledger.com/docs/device-app/develop/ui/ledger-stax-flex-display

@GuilaneDen
Copy link
Contributor

Hey @vforgeoux-ledger , here is the update to use the new NBGL UI useCases.

@vforgeoux-ledger
Copy link
Member

@vforgeoux-ledger
Copy link
Member

Also, I tested the 1.3.0 Celo app version on Flex and :

  • I can't even display the transaction on my device either on Ledger Live or celowallet.app
    telegram-cloud-photo-size-4-6008190408930214908-y
    telegram-cloud-photo-size-4-6008190408930214910-y

  • The app icon on the Flex dashboard isn't the same as the one when the app is opened. It should be not to confuse the users.

  • I noted that the Contract data is ON by default. It's also the case on the prod' version but this kind of setting should be OFF by default.

@keiff3r
Copy link
Contributor

keiff3r commented Oct 10, 2024

Hello @vforgeoux-ledger ,

After investigating the issue with transactions not displaying properly in the Ledger Cello app, I've pinpointed the root cause.

During development, Celo updated the public key used to verify ERC-20 token information. This public key is essential for validating token data, such as those used for fees, and is signed by the new private key held by Celo.

When processing a transaction like CIP-64, it requires ERC-20 token information. This data is provided by a Celo library and needs to be verified using the public key located here. However, since we changed this public key, it seems that the software providing the token information (e.g., ContractKit or wallet-ledger/lib/tokens) hasn’t been updated to reflect this change. As a result, the Ledger device rejects the transaction as it considers the token information invalid.

This is why transactions currently fail on Ledger Live (and likely Celo Wallet). However, looking at the functional tests, you’ll see that CIP-64 transactions pass successfully. This is because, after Celo updated the public key, we requested the properly signed token information using their new private key (see here), which we included in our tests.

I’m not sure which library Ledger Live uses to communicate with the Ledger device, but it likely needs to be updated.

In summary, the app functions as expected, and the library that Ledger Live uses to retrieve ERC-20 token information should be updated. I believe @nicolasbrugneaux can help you identify/fix that issue.

Kind regards.

@aaronmgdr
Copy link

aaronmgdr commented Oct 14, 2024

yes we are working on updating ledger live here LedgerHQ/ledger-live#7622

that will support both the old and new public keys

@vforgeoux-ledger
Copy link
Member

Should we put this app update on hold until the Ledger Live fix hits production @keiff3r @aaronmgdr ?

@keiff3r
Copy link
Contributor

keiff3r commented Oct 15, 2024

I recommend merging this branch into the ‘develop’ branch because the new public key is already in the develop branch (see this commit), so merging this won’t cause further issues; in fact, it will resolve some.

However, I don’t recommend releasing it to users yet, as the update might replace the current working Celo application. Without a software wallet that supports the new public key, users might temporarily lose the ability to transact with Celo (using their ledger) until a compatible wallet is available.

How would Celo like to proceed, @aaronmgdr?

@aaronmgdr
Copy link

@keiff3r 's assessment sounds 100% correct to me. I just managed to get ledger live working with upgrade. although need to do more testing but once done it will support both celo ledger apps that use the new public key and old public key for erc20 data.

This pr is just for flex devices?

there was the previous PR from bloo for celo ledger app to support the new tx types for nano s and nano x ledgers.

Is the release of flex device support and the release of the previous pr tighed together?

What is the time from when this PR is merged to develop till we see the new apps is ledger live store?

@aaronmgdr
Copy link

once this get released celo app will be supported by ledger nano x, ledger nano s, ledger flex, but not Stax right?

asking as im buying all ledger devices that support it

@vforgeoux-ledger
Copy link
Member

Hey @aaronmgdr ,
The upcoming update will bring the content from PR#21 and from this one.

And I can confirm that the app will be supported on all devices including Stax.

@cedelavergne-ledger cedelavergne-ledger merged commit 2fc9524 into LedgerHQ:develop Oct 23, 2024
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants