[SDK] Feature: Add support for forcing legacy transactions in chain configuration #6180
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
size-limit report 📦
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6180 +/- ##
=======================================
Coverage 56.83% 56.84%
=======================================
Files 1168 1168
Lines 64789 64818 +29
Branches 5259 5264 +5
=======================================
+ Hits 36825 36843 +18
- Misses 27237 27248 +11
Partials 727 727
*This pull request uses carry forward flags. Click here to find out more.
|
| /** | ||
| * Whether to force legacy transactions (pre EIP-1559) for this chain | ||
| */ | ||
| forceLegacyTransactions?: boolean; |
There was a problem hiding this comment.
Don't like the boolean because its inflexible, a type: "legacy" might be better (and matches viem/ox APIs)
There was a problem hiding this comment.
Might also want this on the transaction level if the user is providing it
There was a problem hiding this comment.
agreed on both counts. So something like feeType: "legacy" | "eip1559"?
| resolvePromisedValue(transaction.maxFeePerGas), | ||
| resolvePromisedValue(transaction.maxPriorityFeePerGas), | ||
| resolvePromisedValue(transaction.gasPrice), | ||
| resolvePromisedValue(transaction.feeType), |
There was a problem hiding this comment.
so we have it on the chain, on the tx AND in the function itself? seems a bit much no?
There was a problem hiding this comment.
It should come from the chains DB if possible but the user override only on the transaction. User override takes precedence
There was a problem hiding this comment.
for context, I felt this need on engine where I need to expose configuration that allows users to force legacy transactions on a chain. Especially important when engine sends nonce cancellation transactions which are not user triggered. (I could manage this in engine too, but felt like it could be a more general thing so the SDK would benefit from it)
There was a problem hiding this comment.
yeah chain level makes sense to me
| resolvePromisedValue(transaction.maxFeePerGas), | ||
| resolvePromisedValue(transaction.maxPriorityFeePerGas), | ||
| resolvePromisedValue(transaction.gasPrice), | ||
| resolvePromisedValue(transaction.feeType), |
There was a problem hiding this comment.
yeah chain level makes sense to me
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| @@ -40,6 +43,7 @@ const FORCE_GAS_PRICE_CHAIN_IDS = [ | |||
| 1942999413, // Humanity Testnet | |||
| 1952959480, // Lumia Testnet | |||
| 994873017, // Lumia Mainnet | |||
| 1942999413, // Humanity Mainnet | |||
There was a problem hiding this comment.
The chain ID 1942999413 appears twice in FORCE_GAS_PRICE_CHAIN_IDS - once for Humanity Testnet and again for Humanity Mainnet. This appears to be a duplicate entry. If these networks truly have the same chain ID, one entry should be removed. If they are meant to be different networks, one of the chain IDs needs to be corrected.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
looks like the bot is right @d4mr
| @@ -17,6 +17,7 @@ export type StaticPrepareTransactionOptions = { | |||
| maxFeePerGas?: bigint | undefined; | |||
| maxPriorityFeePerGas?: bigint | undefined; | |||
| maxFeePerBlobGas?: bigint | undefined; | |||
| feeType?: FeeType | undefined; | |||
There was a problem hiding this comment.
actually lets not add this, lets just do the chain one.
this prop is too similar the tx.type one which also indicates if its a legacy or 1559 tx
There was a problem hiding this comment.
we still would want a way to tell at the transaction level, hey please calculate the gas and send as legacy. If not this interface, we can use a different one, any thoughts?
There was a problem hiding this comment.
we have type in SerializableTransaction already should map to that
| @@ -40,6 +43,7 @@ const FORCE_GAS_PRICE_CHAIN_IDS = [ | |||
| 1942999413, // Humanity Testnet | |||
| 1952959480, // Lumia Testnet | |||
| 994873017, // Lumia Mainnet | |||
| 1942999413, // Humanity Mainnet | |||
There was a problem hiding this comment.
looks like the bot is right @d4mr
|
This PR has been inactive for 7 days. It is now marked as stale and will be closed in 2 days if no further activity occurs. |
Signed-off-by: Prithvish Baidya <[email protected]>
| } | ||
| // TODO: resolvedFeeType here could be "EIP1559", but we could not get EIP1559 fee data. should we throw? |
There was a problem hiding this comment.
@joaquim-verges wdyt about this btw
There was a problem hiding this comment.
lets not throw, i think there's some cases where both type=1559 + gasprice are valid. polygon being one example IIRC
| } | ||
| // TODO: resolvedFeeType here could be "EIP1559", but we could not get EIP1559 fee data. should we throw? |
There was a problem hiding this comment.
lets not throw, i think there's some cases where both type=1559 + gasprice are valid. polygon being one example IIRC
solves TOOL-3532
PR-Codex overview
This PR introduces enhancements related to transaction types and fee handling in the
thirdwebpackage, adding support for new transaction types and fee structures.Detailed summary
feeTypetoChainOptions.TransactionTypewith various types.StaticPrepareTransactionOptionsto includetype.type.getDefaultGasOverridesto considerfeeType.