chore: find reverted gmp transactions in live mode #12311
Conversation
This file contains hidden or 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
rabi-siddique
force-pushed
the
rs-find-reverted-transactions
branch
2 times, most recently
from
f04b63d to
28b2ce9
Compare
rabi-siddique
changed the title
Contributor
Author
|
Logs from earlier e2e testing specifically if tx is made by the Wallet Owner: Logs when tx is made non-owner of the Wallet: I’ve made many updates since the last round of testing, so I will need to do E2E testing again. |
rabi-siddique
force-pushed
the
rs-find-reverted-transactions
branch
from
5bfdeab to
51dd6d0
Compare
rabi-siddique
force-pushed
the
rs-publish-src-addr
branch
from
74778d6 to
5d58219
Compare
rabi-siddique
force-pushed
the
rs-find-reverted-transactions
branch
from
b9fde86 to
b8b1099
Compare
Contributor
Author
|
E2E testing results Success Case
Revert Case(Authorized User)
|
mergify bot
added a commit
that referenced
this pull request
Jan 12, 2026
…12327) closes: - https://github.com/Agoric/agoric-private/issues/699 ## Description Publishes the expected `sourceAddress` (LCA) for pending GMP transactions into vstorage and updates snapshots accordingly. This enables the GMP watcher to reliably classify reverted Wallet executions by comparing the decoded `sourceAddress` from `calldata` against the expected value, avoiding revert simulation and state-drift issues(see #12311). ### Testing Considerations The vstorage snapshot tests are enough to verify this change. ### Upgrade Considerations - Will require a `ymax0` and `ymax1` update on mainnet.
|
Base branch is changed to master. Please re-run the integration tests by adding 'force:integration' label. |
Contributor
Author
Revert Case(Non-Authorized)
|
rabi-siddique
force-pushed
the
rs-find-reverted-transactions
branch
from
b8b1099 to
183e078
Compare
Muneeb147
reviewed
Jan 12, 2026
| const provider = ctx.evmProviders[caipId] as WebSocketProvider; | ||
|
|
||
| // Extract the address portion from sourceAddress (format: 'cosmos:agoric-3:agoric1...') | ||
| const lcaAddress = parseAccountId(sourceAddress).accountAddress; |
Contributor
There was a problem hiding this comment.
Do we need to assert for lcaAddress presence/correctness? What if accountAddress is not the required format? Or does parseAccountId handle that from inside already (as hard failure)?
Contributor
Author
There was a problem hiding this comment.
It is required and validated in engine.ts via mustMatch
Comment on lines
+269
to
+273
| if (result.settled) { | ||
| const reason = `${logPrefix} Live mode completed`; | ||
| log(reason); | ||
| abortController.abort(reason); | ||
| } |
Contributor
There was a problem hiding this comment.
What about handling tx not settled here?
Comment on lines
+216
to
+223
| return { settled: false }; | ||
| } | ||
|
|
||
| deleteTxBlockLowerBound(kvStore, txId); | ||
| return { found: true, txHash: matchingEvent.transactionHash }; | ||
| return { settled: true, txHash: matchingEvent.transactionHash }; | ||
| } catch (error) { | ||
| log(`Error:`, error); | ||
| return { found: false }; | ||
| return { settled: false }; |
Contributor
There was a problem hiding this comment.
do we need some extra identifier to differentiate b/w not found and error? Because both says settled: false
| let timeoutId: NodeJS.Timeout | undefined; | ||
| let subId: string | null = null; | ||
|
|
||
| const ws = provider.websocket as WebSocket; |
Contributor
There was a problem hiding this comment.
Is this provider.websocket natively provided by alchemy/ethers, right? We don't need to manually connect to it I see?
Contributor
Author
There was a problem hiding this comment.
Yes, provider.websocket is natively provided by ethers.js when using a WebSocketProvider
| reject(WATCH_GMP_ABORTED); | ||
| return; | ||
| } | ||
| if (signal?.aborted) return resolve({ settled: false }); |
Contributor
There was a problem hiding this comment.
Can we add a comment why resolving instead of rejection now?
Contributor
Author
There was a problem hiding this comment.
At the moment, it's the same pattern in all watchers. We don't make use of reject. I think ideally we can refactor later in all watchers to reject with rejectionReason.
| `Watching for MulticallStatus and MulticallExecuted events for txId: ${txId} at contract: ${contractAddress}`, | ||
| ); | ||
| // Named so we can remove it | ||
| const onAbort = () => finish({ settled: false }); |
Contributor
There was a problem hiding this comment.
Do we wanna explicitly log when we aborted signal? Might help in debugging.
Comment on lines
+190
to
+193
| ws.off('message', messageHandler); | ||
| ws.off('error', onWsError); | ||
| ws.off('close', onWsClose); | ||
| signal?.removeEventListener('abort', onAbort); |
Contributor
There was a problem hiding this comment.
Add a log say: "Cleaning up ws and listeners"..?
Comment on lines
+306
to
+316
| const subscribe = async () => { | ||
| await provider.getNetwork(); | ||
|
|
||
| subId = await provider.send('eth_subscribe', [ | ||
| 'alchemy_minedTransactions', | ||
| { | ||
| addresses: [{ to: contractAddress }], | ||
| includeRemoved: false, | ||
| hashesOnly: false, | ||
| }, | ||
| ]); |
Contributor
There was a problem hiding this comment.
Can we handle the failure to subscribe here? Or does the caller handle that in catch?
Contributor
Author
There was a problem hiding this comment.
It's happening via .catch(fail) on line 327
Comment on lines
+334
to
+338
| if (!done) { | ||
| log( | ||
| `✗ No MulticallStatus or MulticallExecuted found for txId ${txId} within ${timeoutMs / 60000} minutes`, | ||
| ); | ||
| const { status, errorMessage } = await findTxStatusFromAxelarscan( | ||
| txId, | ||
| contractAddress, | ||
| { | ||
| axelarApiUrl, | ||
| fetch, | ||
| }, | ||
| `✗ No transaction status found for txId ${txId} within ${ | ||
| timeoutMs / 60000 | ||
| } minutes`, |
Contributor
There was a problem hiding this comment.
I see we make done = true on failure as well. So done essentially means that tx is processed? Either success/failed?
Contributor
Author
There was a problem hiding this comment.
No, done means the watcher is finished (stopped watching), regardless of whether the transaction succeeded, failed, or was never observed.
gibson042
reviewed
Jan 13, 2026
|
|
||
| // Custom error code for aborted GMP watch | ||
| export const WATCH_GMP_ABORTED = 'WATCH_GMP_ABORTED'; | ||
| // Alchemy alchemy_minedTransactions subscription message types |
Member
There was a problem hiding this comment.
Please link to documentation if possible, and feel free to use //#region///#endregion.
| fetch: typeof fetch; | ||
| axelarApiUrl: string; | ||
| }; | ||
| // AxelarExecutable entrypoint (standard) |
Member
There was a problem hiding this comment.
Please link to documentation if possible.
Comment on lines
+69
to
+71
| const WALLET_EXECUTE_ABI = [ | ||
| 'function execute(bytes32 commandId, string sourceChain, string sourceAddress, bytes payload) external', | ||
| ]; |
Member
There was a problem hiding this comment.
GMP_ABI uses the same _ABI name suffix but the JSON ABI representation, and I think particularly the inputs of a function... can we commit to and document a particular convention? I wouldn't mind if it's verbose, e.g. GMP_INPUTS_ABI_JSON and WALLET_EXECUTE_CONTRACT_ABI_TEXT.
Comment on lines
+88
to
+91
| // execute(bytes32 commandId, string sourceChain, string sourceAddress, bytes payload) | ||
| const sourceAddress: string = parsed.args?.[2]; | ||
| const payload: string = parsed.args?.[3]; | ||
| if (!sourceAddress || !payload) return null; |
Member
There was a problem hiding this comment.
Suggested change
| // execute(bytes32 commandId, string sourceChain, string sourceAddress, bytes payload) | |
| const sourceAddress: string = parsed.args?.[2]; | |
| const payload: string = parsed.args?.[3]; | |
| if (!sourceAddress || !payload) return null; | |
| const [_commandId, _sourceChain, sourceAddress, payload] = parsed.args; | |
| if (!sourceAddress || !payload) return null; |
Comment on lines
+93
to
+100
| // CallMessage { string id; ContractCalls[] calls; } | ||
| const decoded = abiCoder.decode( | ||
| ['tuple(string id, tuple(address target, bytes data)[] calls)'], | ||
| payload, | ||
| ); | ||
|
|
||
| const txId = decoded?.[0]?.id; | ||
| if (typeof txId !== 'string') return null; |
Member
There was a problem hiding this comment.
Suggested change
| // CallMessage { string id; ContractCalls[] calls; } | |
| const decoded = abiCoder.decode( | |
| ['tuple(string id, tuple(address target, bytes data)[] calls)'], | |
| payload, | |
| ); | |
| const txId = decoded?.[0]?.id; | |
| if (typeof txId !== 'string') return null; | |
| // CallMessage { string id; ContractCalls[] calls; } | |
| const [decoded] = abiCoder.decode( | |
| ['tuple(string id, tuple(address target, bytes data)[] calls)'], | |
| payload, | |
| ); | |
| const txId = decoded?.id; | |
| if (typeof txId !== 'string') return null; |
Comment on lines
+276
to
+297
| if (receipt.status === 1 && matchingLog) { | ||
| log( | ||
| `✅ SUCCESS: txId=${txId} txHash=${txHash} block=${receipt.blockNumber}`, | ||
| ); | ||
| return finish({ settled: true, txHash }); | ||
| } | ||
|
|
||
| if (receipt.status === 0) { | ||
| /** | ||
| * Transaction reverted - since we've already validated that the sourceAddress | ||
| * matches our expected LCA address, this is a legitimate execution attempt | ||
| * from our own wallet that failed. We treat this as a transaction failure. | ||
| * | ||
| * Note: Spurious executions from unauthorized parties are already filtered | ||
| * out by the sourceAddress check above, so any revert we see here represents | ||
| * a genuine failure of the user's operation. | ||
| */ | ||
| log( | ||
| `❌ REVERTED: txId=${txId} txHash=${txHash} block=${receipt.blockNumber} - transaction failed`, | ||
| ); | ||
| return finish({ settled: true, txHash }); | ||
| } |
Member
There was a problem hiding this comment.
I find it troubling that these two cases collapse into indistinguishable output.
Comment on lines
+309
to
+318
| subId = await provider.send('eth_subscribe', [ | ||
| 'alchemy_minedTransactions', | ||
| { | ||
| addresses: [{ to: contractAddress }], | ||
| includeRemoved: false, | ||
| hashesOnly: false, | ||
| }, | ||
| ]); | ||
|
|
||
| ws.on('message', messageHandler); |
Member
There was a problem hiding this comment.
Is there any chance of a race here? Our general pattern is to register listeners before creating circumstances that could cause them to be invoked.
Comment on lines
+321
to
+324
| // Attach listeners (all removable) | ||
| ws.on('error', onWsError); | ||
| ws.on('close', onWsClose); | ||
| signal?.addEventListener('abort', onAbort); |
Comment on lines
345
to
347
| export const EVENTS = { | ||
| MULTICALL_EXECUTED: 'executed', | ||
| MULTICALL_STATUS: 'status', | ||
| }; |
Member
There was a problem hiding this comment.
Seems like the EVENTS container is now superfluous.
|
|
||
| // Mock websocket for gmp-watcher - store event handlers | ||
| const wsEventHandlers = new Map<string, Function[]>(); | ||
| const mockWebSocket = { |
Member
There was a problem hiding this comment.
This seems to be hand-rolling a partial implementation of EventEmitter; why not just use what's already provided?
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.
closes:
Description
Removes the dependency on AxelarScan for GMP transaction failure status detection. Instead, it now uses Alchemy WebSocket connections to detect failed GMP transactions on EVM. Alchemy WebSockets are used for both failure and success detection.
This functionality is currently implemented in the live mode of the GMP watcher. Support for lookback mode will be added in a subsequent PR, as detecting reverted transactions in historical logs requires a different mechanism.
Testing Considerations
Existing tests have been adapted to the new changes. Mocks were updated as well. And couple of new tests were added for testing revert behavior.
Upgrade Considerations