fix: bump hdwallet package versions to fix walletconnect txs for evm chains #5425
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
woodenfurniture
commented
Oct 6, 2023
woodenfurniture
commented
Oct 6, 2023
woodenfurniture
commented
Oct 6, 2023
woodenfurniture
commented
Oct 6, 2023
woodenfurniture
commented
Oct 6, 2023
There was a problem hiding this comment.
Conceptual stamps before hdwallet packages are published.
Tested locally with locally published packages and confirmed this does what it says on the box.
ethSwitchChain()works with Trust, tested by switching chains in the chain switch menu. Note, this is transparent to the user here, so it may look like nothing is happening, but definitely is.- Approvals are using the right EVM chain, e.g when making a trade on BSC while being connected to mainnet, the approval is happening on BSC
- Same as above for trade execution
Note, the hdwallet PR (and consequently, this consumption) effectively break 1inch wc when a chain switch is needed, which is not an us concern. Gut feel is the endpoint isn't properly handled on 1inch side, which ends up returning an internal error.
gomesalexandre
approved these changes
Oct 9, 2023
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Resolves app attempting to broadcast non-eth-mainnet evm transactions on eth-mainnet, resulting in failed transaction broadcast in the case of trades, and incorrect approvals being sent on the wrong chain.
NOTE: This requires hdwallet changes to work (link to come when i have access)
See here for details:
#5421 (comment)
Pull Request Type
Issue (if applicable)
closes #5421
Risk
Risk of lost funds and broken trading + approvals when using a wallet connected to app via walletconnect.
Testing
Using a wallet connected via walletconnect:
ERC20 allowance approvals on eth mainnet
ERC20 -> ERC20 trade on eth mainnet
ETH -> ERC20 trade on eth mainnet
ERC20 -> ETH trade on eth mainnet
ERC20 allowance approvals on avalanche/polygon/gnosis/bnbsmartchain
ERC20 -> ERC20 trade on avalanche/polygon/gnosis/bnbsmartchain
ETH -> ERC20 trade on avalanche/polygon/gnosis/bnbsmartchain
ERC20 -> ETH trade on avalanche/polygon/gnosis/bnbsmartchain
Engineering
This requires hdwallet changes to work
Operations
☝️
Screenshots (if applicable)
DAI on avalanche approval before and after the fix:
