-
Notifications
You must be signed in to change notification settings - Fork 7
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
Upgrade celo deps #227
Upgrade celo deps #227
Conversation
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. Ignoring: Next stepsTake a deeper look at the dependencyTake 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 packageIf 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 riskTo ignore an alert, reply with a comment starting with |
189ca49
to
6e07d1a
Compare
dbcc061
to
6aeb878
Compare
2002b97
to
71c50ed
Compare
@SocketSecurity ignore npm/[email protected] |
This is on hold until we figure out if we want to do the changes required in contractkit or here. |
@palango Do you mind adding more context on why are these changes being made, consequences of not adding them, timelines, etc? |
strongly advice merging this as its upgrading very very very old dependencies. |
@SocketSecurity ignore npm/[email protected] |
This reverts commit 2a95b30.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would have said in description that it also moves to pnpm (besides the upgrade) but code seems good
ps only reviewed as there was a request specifically for me to
@bowd @sissnad @nvtaveras From our side this is ready to be merged, but we'd like to have some review from you as well. Regarding the rollout, can we help with that somehow or will you talk to the oracle operators? |
Hey @palango, we've been busy lately with other work but would like to deploy this to one of the testnets and have them run there for a bit before merging. Are you fine holding off until then? My current guess is that we will have capacity to test this in around two weeks from now |
@nvtaveras Thanks for the update. |
@SocketSecurity ignore npm/[email protected] |
@SocketSecurity ignore npm/[email protected] |
@palango Sure, go for it, but please do it in Baklava instead, as Alfajores it's supposed to be the stable testnet, in case something breaks :) Also feel free to reach out on discord if you need help |
We're running this branch on baklava now. Everything seems to work fine and the transactions are now eth compatible. |
This is running fine since last Friday, therefore merging. |
Description
The main focus of this PR is updating
@celo/contractkit
and related packages, in order to not send deprecated legacy celo transactions.In prior versions, contractkit sends legacy celo transactions when just the gas price is passed as is the case here. @carterqw2 updated contractkit to send legacy ethereum transactions instead.
In future updates it might make sense to update the oracle to send EIP-1559 transactions.
Some changes include upgrading node and the dockerfiles.
Tested
yarn test
Backwards compatibility
Yes