Fix legacy transactions requests to signAndSendTransaction in core
#1096
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 560338c:
|
.changeset/lemon-breads-win.md
Outdated
| "@turnkey/core": patch | ||
| --- | ||
|
|
||
| Fixed legacy transaction usage in signAndSendTransaction, you can now pass in proper gas fees when using legacy tx |
There was a problem hiding this comment.
Let's add some more context on which specific path this affects -- to my understanding this specifically affects the pass-through external wallet signing flow, but not the Turnkey (embedded wallet) flow. Is this accurate?
There was a problem hiding this comment.
Yep, this should only affect connected wallets, will update!
| ...base, | ||
| gasPrice: toHex(tx.gasPrice), | ||
| }; | ||
| } else { |
There was a problem hiding this comment.
Unlikely that this happens, but I'd ideally like to add explicit checks for the known tx types: https://reth.rs/run/faq/transactions/
in other words, if it's not in the range of 0-4, we should error that it's an unknown type
There was a problem hiding this comment.
Ok, unfortunately there is a bunch to dig into here, mainly differences in how ethers and viem build their transaction objects and how we need to handle it in our sign function so me & Moe O decided its best to table this for now and figure it all out later so we're not blocking customers
There was a problem hiding this comment.
+1 - I need to dig into this more, but right now it doesn’t seem reasonable for us to reliably handle all cases when going from:
serializedTransaction -> Transaction object
versus the safer direction of:
Transaction object -> serializedTransaction
| if (txType === 0 || txType === 1) { | ||
| // legacy or EIP-2930 (gasPrice-based) | ||
| if (tx.gasPrice == null) { | ||
| throw new Error("Legacy or EIP-2930 transaction missing gasPrice"); |
There was a problem hiding this comment.
Just as an FYI -- while Turnkey supports legacy transactions, they still have to be EIP-155 compliant (https://eips.ethereum.org/EIPS/eip-155). But this only affects transactions signed using Turnkey/embedded wallets. I believe this codepath uses browser wallets, so shouldn't be a huge issue here
moeodeh3
left a comment
moeodeh3
left a comment
There was a problem hiding this comment.
WalletConnect constructs the transaction for EVM the same way (i.e has the same problem)
can we also copy over the logic there as well 🙏
wallet-connect/base.ts
| /** @internal */ | ||
| export type Wallet = EmbeddedWallet | ConnectedWallet; | ||
|
|
||
| export type BaseTransactionParams = { |
There was a problem hiding this comment.
two things:
- these types are shared between Solana and Evm, so lets make sure we name it in a way we know this is used only for EVM
- lets avoid naming things "Base" since that conflicts with Base network
thinking to name it instead something like EvmTransactionParams
| maxPriorityFeePerGas: `0x${string}`; | ||
| }; | ||
|
|
||
| export type LegacyTransactionParams = BaseTransactionParams & { |
There was a problem hiding this comment.
same thing here, lets make sure to name this in an EVM specific way - so something like LegacyEvmTransactionParams
There was a problem hiding this comment.
or EvmLegacyTransactionParams
.changeset/lemon-breads-win.md
Outdated
| "@turnkey/core": patch | ||
| --- | ||
|
|
||
| Fixed legacy transaction usage in signAndSendTransaction for **Connected wallets**. This does not affect Turnkey's embedded wallet flow, previously, connected wallet transactions were all formatted into EIP-1559 transactions, updated to respect legacy + future formats passed in. |
There was a problem hiding this comment.
"Fixed legacy transactions not working in signAndSendTransaction() for Evm connected wallet"
Summary & Motivation
How I Tested These Changes
Did you add a changeset?
If updating one of our packages, you'll likely need to add a changeset to your PR. To do so, run
pnpm changeset.pnpm changesetwill generate a file where you should write a human friendly message about the changes. Note how this (example) includes the package name (should be auto added by the command) along with the type of semver change (major.minor.patch) (which you should set).These changes will be used at release time to determine what packages to publish and how to bump their version. For more context see this comment.