-
-
Notifications
You must be signed in to change notification settings - Fork 267
fix: set isExternalSign to true for delegated transactions #7659
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
base: main
Are you sure you want to change the base?
Changes from all commits
d47cb49
3314e54
a56c32c
0b8b218
30086f3
c1ba9c1
1d3d23a
99b756f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1351,6 +1351,8 @@ export class TransactionController extends BaseController< | |
| const existingTransactionMeta = this.#getTransactionWithActionId(actionId); | ||
|
|
||
| // If a request to add a transaction with the same actionId is submitted again, a new transaction will not be created for it. | ||
| // EIP-7702 batch transactions (with nestedTransactions) are signed externally via delegation | ||
| const isEIP7702Batch = Boolean(nestedTransactions?.length); | ||
| let addedTransactionMeta: TransactionMeta = existingTransactionMeta | ||
| ? cloneDeep(existingTransactionMeta) | ||
| : { | ||
|
|
@@ -1363,6 +1365,7 @@ export class TransactionController extends BaseController< | |
| deviceConfirmedOn, | ||
| disableGasBuffer, | ||
| id: random(), | ||
| isExternalSign: isEIP7702Batch, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, this is the same issue as before. EIP-7702 batch transactions can still be signed locally by the EOA itself and more typically are. The only exceptions to that are when the gas station EIP-7702 flow is used as it's signed by our relay. If we're missing a scenario when a transaction is signed externally, then we need to update |
||
| isGasFeeTokenIgnoredIfBalance: Boolean(gasFeeToken), | ||
| isGasFeeIncluded, | ||
| isGasFeeSponsored, | ||
|
|
@@ -1457,6 +1460,8 @@ export class TransactionController extends BaseController< | |
|
|
||
| this.#addMetadata(addedTransactionMeta); | ||
|
|
||
| // Record delegationAddress for metrics, but do not use it to determine isExternalSign. | ||
| // Only EIP-7702 batch transactions (with nestedTransactions) should have isExternalSign = true. | ||
| delegationAddressPromise | ||
| .then((delegationAddress) => { | ||
| this.#updateTransactionInternal( | ||
|
|
||
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.
EIP-7702 batch
isExternalSignoverride by gas fee token checkMedium Severity
The new
isExternalSign = isEIP7702Batchcan be overridden bycheckGasFeeTokenBeforePublishwhen a transaction has bothnestedTransactionsand agasFeeToken, and the user has sufficient native balance. ThecheckGasFeeTokenBeforePublishfunction unconditionally setsisExternalSign = falsein this case, which would cause the original nonce conflicts for EIP-7702 batch transactions that the PR aims to fix.