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

Prevent Broken ledger live for celo users #7622

Draft
wants to merge 24 commits into
base: develop
Choose a base branch
from

Conversation

aaronmgdr
Copy link

@aaronmgdr aaronmgdr commented Aug 19, 2024

✅ Checklist

  • npx changeset was attached.
  • Covered by automatic tests. => dependency update no function change
  • Impact of the changes:
    • celo specific features. such as voting for validators, and staking

📝 Description

It is necessary to upgrade celo dependencies for ledger live to continue working with Celo once the Celo transition to Layer 2 happens. This is because early versions of celo blockchain used a custom transaction type and older versions of celo sdks only build transictions using this deprecated transaction format. However cel2 will not support this transaction format.

❓ Context

closes #7551


🧐 Checklist for the PR Reviewers

  • The code aligns with the requirements described in the linked JIRA or GitHub issue.
  • The PR description clearly documents the changes made and explains any technical trade-offs or design decisions.
  • There are no undocumented trade-offs, technical debt, or maintainability issues.
  • The PR has been tested thoroughly, and any potential edge cases have been considered and handled.
  • Any new dependencies have been justified and documented.
  • Performance considerations have been taken into account. (changes have been profiled or benchmarked if necessary)

@live-github-bot live-github-bot bot added common Has changes in live-common fork Pull request base branch comes from a fork. labels Aug 19, 2024
Copy link

vercel bot commented Aug 19, 2024

@aaronmgdr is attempting to deploy a commit to the LedgerHQ Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

vercel bot commented Aug 19, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
web-tools ❌ Failed (Inspect) Nov 12, 2024 0:05am
3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
ledger-live-docs ⬜️ Ignored (Inspect) Visit Preview Nov 12, 2024 0:05am
native-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Nov 12, 2024 0:05am
react-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Nov 12, 2024 0:05am

Copy link

socket-security bot commented Aug 19, 2024

🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎

To accept the risk, merge this PR and you will not be notified again.

Alert Package NoteSourceCI
Git dependency npm/@celo/[email protected] ⚠︎

View full report↗︎

Next steps

What are git dependencies?

Contains a dependency which resolves to a remote git URL. Dependencies fetched from git URLs are not immutable can be used to inject untrusted code or reduce the likelihood of a reproducible install.

Publish the git dependency to npm or a private package repository and consume it from there.

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/[email protected] or ignore all packages with @SocketSecurity ignore-all

@@ -62,20 +62,6 @@ function readPackage(pkg, context) {
addPeerDependencies("@storybook/addon-react-native-web", {
webpack: "*",
}),
/* @celo/* packages */
addDependencies(/@celo\/(?!base)+/, { "@celo/base": `^${pkg.version}` }),
Copy link
Author

@aaronmgdr aaronmgdr Aug 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is no longer necessary and in fact breaks install as celo deps no longer locked together their versions

@aaronmgdr
Copy link
Author

perplexed as to why in the ledger live environment calling toChecksumAddress results in eventually the @noble/hash library asserting that the Buffer of the address is a Uint8Array and (this is the crucial part) it gives false. whereas i can trace the same code path in a different environment and it will return true. I made an empty electron repo with just ContractKit as I thought it could be the differenc but received no issue.

@aaronmgdr
Copy link
Author

This fails because in JSDom Buffer does not inherit from Uint8Array. this causes a cascade of failures starting in @noble/hashes and bubbling up to web3.js new Contract() which is called from newKit (from contractkit)

Its a bit of a bummer. And im not sure of the best approach to fix it.

As an alternative I played around with removing contractkit completely. But that too requires a bit more to get it working, and will change a substantial amount of files. So i wont do more on that just yet.

@aaronmgdr
Copy link
Author

@Wozacosta id love to talk more about how we can get this working.

Copy link

There as been no activity on this PR for the last 14 days. Please consider closing this PR.

@github-actions github-actions bot added the Stale label Sep 13, 2024
@aaronmgdr
Copy link
Author

do not close this pr

@aaronmgdr aaronmgdr changed the title Upgrade Celo Dependencies Prevent Broken ledger live for celo users Sep 13, 2024
@github-actions github-actions bot removed the Stale label Sep 14, 2024
@live-github-bot live-github-bot bot added the desktop Has changes in LLD label Sep 17, 2024
@qperrot
Copy link
Contributor

qperrot commented Sep 27, 2024

Hello @aaronmgdr,
I am trying your PR* locally, and have issues when trying to do any actions using my Celo accounts (sending, withdrawing rewards).
Did you face the same issue on your side ?
Here a screen shot of the error:
Screenshot 2024-09-27 at 10 16 53

Also, the @celo packages that are being updated have this error message: Versions less than 5.1 are deprecated and will no longer be able to submit transactions to celo in a future hardfork
Do you have a timeline or a date that we should have in mind to judge of the urgency.

@aaronmgdr
Copy link
Author

Hello @aaronmgdr, I am trying your PR* locally, and have issues when trying to do any actions using my Celo accounts (sending, withdrawing rewards). Did you face the same issue on your side ? Here a screen shot of the error: Screenshot 2024-09-27 at 10 16 53

Also, the @celo packages that are being updated have this error message: Versions less than 5.1 are deprecated and will no longer be able to submit transactions to celo in a future hardfork Do you have a timeline or a date that we should have in mind to judge of the urgency.

@qperrot thanks for getting back. to be honest i had trouble understanding how to get the ledger system running. Hoping we can work together on this. It was tricky upgrade so far.

As for the timeline mentioned. It is the same as the celo transition to L2. The Testnet was just upgraded today and the plan for celo mainnet is by end of year

@aaronmgdr
Copy link
Author

@aaronmgdr
Copy link
Author

same as #8240

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
common Has changes in live-common desktop Has changes in LLD fork Pull request base branch comes from a fork.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Outdated celo dependencies
4 participants