[work-in-progress] improve manual relay #245
Merged
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
canhtrinh
changed the title
milapsheth
reviewed
Feb 10, 2023
| @@ -97,6 +104,9 @@ export class AxelarGMPRecoveryAPI extends AxelarRecoveryApi { | |||
| signCommandTx, | |||
| }); | |||
There was a problem hiding this comment.
unused create pending transfer variable above
| @@ -97,6 +104,9 @@ export class AxelarGMPRecoveryAPI extends AxelarRecoveryApi { | |||
| signCommandTx, | |||
| }); | |||
|
|
|||
There was a problem hiding this comment.
unused create pending transfer variable above
| @@ -0,0 +1,15 @@ | |||
| import { arrayify, keccak256 } from "ethers/lib/utils"; | |||
|
|
|||
| export function getCommandId(chainID: number, txHash: string, sourceEventIndex: number) { | |||
There was a problem hiding this comment.
add a link to core for the derivation?
| @@ -173,14 +181,15 @@ export function findContractEvent( | |||
| eventSignatures: string[], | |||
| abiInterface: Interface | |||
| ): Nullable<EventLog> { | |||
| for (const log of receipt.logs) { | |||
| for (const [index, log] of receipt.logs.entries()) { | |||
| const eventIndex = eventSignatures.indexOf(log.topics[0]); | |||
| if (eventIndex > -1) { | |||
| const eventLog = abiInterface.parseLog(log); | |||
| return { | |||
| signature: eventSignatures[eventIndex], | |||
| eventLog, | |||
| logIndex: log.logIndex, | |||
There was a problem hiding this comment.
logIndex is the event index, right?
| const batchData = await this.fetchBatchData(commandId); | ||
| if (batchData) { | ||
| const command = batchData.commands.find((command) => command.id === commandId); | ||
| if (command && !command?.executed) { |
There was a problem hiding this comment.
not very readable, should encapsulate in a separate function, and return early on the failure condition for less nested ifs
|
|
||
| if (!signEvent) return errorResponse(ApproveGatewayError.SIGN_COMMAND_FAILED); | ||
|
|
||
| await sleep(2); | ||
| const batchedCommandId = signEvent.attributes.find( |
There was a problem hiding this comment.
sign commands is racing axelarons here, so you will often not get the correct batch id to relay. The logic is also duplicated. I think this should be split across a few functions, and look something like:
- Add a method to check if tx hash is confirmed. Use the event query for this.
- If the event status is failed, abort.
- If not confirmed yet, check if tx is finalized (this could be skipped if axelarscan only shows the Approve button after src tx is finalized, to keep the logic here simpler, otherwise this method should check if latest block is x block confirmations over tx).
- And then call the confirm gateway tx method. Wait for 30s for votes to go through (can take 2-3 blocks) and signing/relaying to occur.
- Check if event is in completed status now. Fail if not.
- if event was in completed status, 2-5 can be skipped.
- If destination chain is not EVM, fail (with cosmos gmp this will change).
- Check if command is executed.
- If the batch itself wasn't found, then sign and wait 20s
- If the batch wasn't found OR command wasn't executed, submit batch to gateway using user's wallet, and wait for 20s.
- If command is still not executed, return an error.
Merged
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.
this PR fixes #175.
the issue was in the
manualRelayToDestChainmethod, which was eagerly attempting to confirm every tx hash without first checking whether the tx has already been confirmed on the network.the issue that comes up with
cannot sign commandcomes up for transactions that were already confirmed but the batch has not been broadcasted to the destination chain.this fix: