-
Notifications
You must be signed in to change notification settings - Fork 20
feature: Get all Invalid Fills #1085
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: master
Are you sure you want to change the base?
Changes from 4 commits
1b82a73
1b8aaed
4a67c15
9727794
a54510a
e6c9da3
64b60e3
02c4d33
64bcaf4
5bb97f1
f9d939e
1be82ad
be74e90
9f8a633
4f60a53
276d648
4fa6106
cc1dfad
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 |
---|---|---|
|
@@ -7,7 +7,7 @@ import { | |
relayFillStatus, | ||
getTimestampForBlock as _getTimestampForBlock, | ||
} from "../../arch/evm"; | ||
import { DepositWithBlock, FillStatus, RelayData } from "../../interfaces"; | ||
import { DepositWithBlock, FillStatus, Log, RelayData } from "../../interfaces"; | ||
import { | ||
BigNumber, | ||
DepositSearchResult, | ||
|
@@ -17,6 +17,7 @@ import { | |
MakeOptional, | ||
toBN, | ||
EvmAddress, | ||
MultipleDepositSearchResult, | ||
} from "../../utils"; | ||
import { | ||
EventSearchConfig, | ||
|
@@ -146,28 +147,25 @@ export class EVMSpokePoolClient extends SpokePoolClient { | |
return _getTimeAt(this.spokePool, blockNumber); | ||
} | ||
|
||
public override async findDeposit(depositId: BigNumber): Promise<DepositSearchResult> { | ||
let deposit = this.getDeposit(depositId); | ||
if (deposit) { | ||
return { found: true, deposit }; | ||
} | ||
|
||
// No deposit found; revert to searching for it. | ||
const upperBound = this.latestHeightSearched || undefined; // Don't permit block 0 as the high block. | ||
private async queryDepositEvents( | ||
depositId: BigNumber | ||
): Promise<{ events: Log[]; from: number; chain: string; elapsedMs: number } | { reason: string }> { | ||
pxrl marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const tStart = Date.now(); | ||
const upperBound = this.latestHeightSearched || undefined; | ||
const from = await findDepositBlock(this.spokePool, depositId, this.deploymentBlock, upperBound); | ||
const chain = getNetworkName(this.chainId); | ||
|
||
if (!from) { | ||
const reason = | ||
`Unable to find ${chain} depositId ${depositId}` + | ||
` within blocks [${this.deploymentBlock}, ${upperBound ?? "latest"}].`; | ||
return { found: false, code: InvalidFill.DepositIdNotFound, reason }; | ||
return { | ||
reason: `Unable to find ${chain} depositId ${depositId} within blocks [${this.deploymentBlock}, ${ | ||
upperBound ?? "latest" | ||
}].`, | ||
}; | ||
pxrl marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
const to = from; | ||
const tStart = Date.now(); | ||
// Check both V3FundsDeposited and FundsDeposited events to look for a specified depositId. | ||
const { maxLookBack } = this.eventSearchConfig; | ||
const query = ( | ||
const events = ( | ||
await Promise.all([ | ||
paginatedEventQuery( | ||
this.spokePool, | ||
|
@@ -181,7 +179,25 @@ export class EVMSpokePoolClient extends SpokePoolClient { | |
), | ||
]) | ||
).flat(); | ||
|
||
const tStop = Date.now(); | ||
return { events, from, chain, elapsedMs: tStop - tStart }; | ||
} | ||
|
||
public override async findDeposit(depositId: BigNumber): Promise<DepositSearchResult> { | ||
let deposit = this.getDeposit(depositId); | ||
if (deposit) { | ||
return { found: true, deposit }; | ||
} | ||
|
||
// No deposit found; revert to searching for it. | ||
const result = await this.queryDepositEvents(depositId); | ||
|
||
if ("reason" in result) { | ||
return { found: false, code: InvalidFill.DepositIdNotFound, reason: result.reason }; | ||
} | ||
|
||
const { events: query, from, chain, elapsedMs } = result; | ||
|
||
const event = query.find(({ args }) => args["depositId"].eq(depositId)); | ||
if (event === undefined) { | ||
|
@@ -210,12 +226,74 @@ export class EVMSpokePoolClient extends SpokePoolClient { | |
at: "SpokePoolClient#findDeposit", | ||
message: "Located deposit outside of SpokePoolClient's search range", | ||
deposit, | ||
elapsedMs: tStop - tStart, | ||
elapsedMs, | ||
}); | ||
|
||
return { found: true, deposit }; | ||
} | ||
|
||
public override async findAllDeposits(depositId: BigNumber): Promise<MultipleDepositSearchResult> { | ||
// First check memory for deposits | ||
let deposits = this.getDepositsForDepositId(depositId); | ||
if (deposits.length > 0) { | ||
return { found: true, deposits: deposits }; | ||
pxrl marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
// If no deposits found in memory, try to find on-chain | ||
const result = await this.queryDepositEvents(depositId); | ||
if ("reason" in result) { | ||
return { found: false, code: InvalidFill.DepositIdNotFound, reason: result.reason }; | ||
} | ||
|
||
const { events: query, chain, elapsedMs } = result; | ||
|
||
// Find all events with matching depositId | ||
const events = query.filter(({ args }) => args["depositId"].eq(depositId)); | ||
pxrl marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
if (events.length === 0) { | ||
return { | ||
found: false, | ||
code: InvalidFill.DepositIdNotFound, | ||
reason: `${chain} depositId ${depositId} not found at block ${result.from}.`, | ||
}; | ||
} | ||
|
||
// First do all synchronous operations | ||
deposits = events.map((event) => { | ||
const deposit = { | ||
...spreadEventWithBlockNumber(event), | ||
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. This PR looks fine to me. I'd prefer if we can merge this after we do the address migration though. Once we do that, you'd need to manually cast the address args here (like |
||
originChainId: this.chainId, | ||
fromLiteChain: true, | ||
toLiteChain: true, | ||
} as DepositWithBlock; | ||
|
||
if (isZeroAddress(deposit.outputToken)) { | ||
deposit.outputToken = this.getDestinationTokenForDeposit(deposit); | ||
} | ||
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. fwiw I think this needs to be re-worked following the Address class updates that were merged on Friday. 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. |
||
deposit.fromLiteChain = this.isOriginLiteChain(deposit); | ||
deposit.toLiteChain = this.isDestinationLiteChain(deposit); | ||
|
||
return deposit; | ||
}); | ||
|
||
// Then do all async operations in parallel | ||
deposits = await Promise.all( | ||
deposits.map(async (deposit) => ({ | ||
...deposit, | ||
quoteBlockNumber: await this.getBlockNumber(Number(deposit.quoteTimestamp)), | ||
})) | ||
); | ||
pxrl marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
this.logger.debug({ | ||
at: "SpokePoolClient#findAllDeposits", | ||
message: "Located deposits outside of SpokePoolClient's search range", | ||
deposits: deposits, | ||
elapsedMs, | ||
}); | ||
|
||
return { found: true, deposits: deposits }; | ||
pxrl marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
public override getTimestampForBlock(blockNumber: number): Promise<number> { | ||
return _getTimestampForBlock(this.spokePool.provider, blockNumber); | ||
} | ||
|
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. @melisaguevara @bmzig You're both strong on the SVMSpokePoolClient - possible to take a pass over this file? |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,20 +11,22 @@ import { | |
relayFillStatus, | ||
fillStatusArray, | ||
} from "../../arch/svm"; | ||
import { FillStatus, RelayData, SortableEvent } from "../../interfaces"; | ||
import { DepositWithBlock, FillStatus, RelayData, SortableEvent } from "../../interfaces"; | ||
import { | ||
BigNumber, | ||
DepositSearchResult, | ||
EventSearchConfig, | ||
InvalidFill, | ||
isZeroAddress, | ||
MakeOptional, | ||
MultipleDepositSearchResult, | ||
sortEventsAscendingInPlace, | ||
SvmAddress, | ||
} from "../../utils"; | ||
import { isUpdateFailureReason } from "../BaseAbstractClient"; | ||
import { HubPoolClient } from "../HubPoolClient"; | ||
import { knownEventNames, SpokePoolClient, SpokePoolUpdate } from "./SpokePoolClient"; | ||
import { findAllDeposits } from "../../arch/svm/SpokeUtils"; | ||
|
||
/** | ||
* SvmSpokePoolClient is a client for the SVM SpokePool program. It extends the base SpokePoolClient | ||
|
@@ -231,6 +233,41 @@ export class SVMSpokePoolClient extends SpokePoolClient { | |
}; | ||
} | ||
|
||
public override async findAllDeposits(depositId: BigNumber): Promise<MultipleDepositSearchResult> { | ||
// TODO: Should we have something like this? In findDeposit we don't look in memory. | ||
// // First check memory for deposits | ||
// const memoryDeposits = this.getDepositsForDepositId(depositId); | ||
// if (memoryDeposits.length > 0) { | ||
// return { found: true, deposits: memoryDeposits }; | ||
// } | ||
|
||
// If no deposits found in memory, try to find on-chain | ||
const deposits = await findAllDeposits(this.svmEventsClient, depositId); | ||
if (!deposits || deposits.length === 0) { | ||
return { | ||
found: false, | ||
code: InvalidFill.DepositIdNotFound, | ||
reason: `Deposit with ID ${depositId} not found`, | ||
pxrl marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}; | ||
} | ||
|
||
// Enrich all deposits with additional information | ||
const enrichedDeposits = await Promise.all( | ||
deposits.map(async (deposit: DepositWithBlock) => ({ | ||
...deposit, | ||
quoteBlockNumber: await this.getBlockNumber(Number(deposit.quoteTimestamp)), | ||
pxrl marked this conversation as resolved.
Show resolved
Hide resolved
|
||
originChainId: this.chainId, | ||
fromLiteChain: this.isOriginLiteChain(deposit), | ||
toLiteChain: this.isDestinationLiteChain(deposit), | ||
outputToken: isZeroAddress(deposit.outputToken) | ||
? this.getDestinationTokenForDeposit(deposit) | ||
: deposit.outputToken, | ||
})) | ||
); | ||
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. @dijanin-brat ooc, should this be 1:1 with the EVM implementation? 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. They work with 2 different types of data (EVM has 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. For follow-up after this PR, yeah. I'm pretty sure they should using the same underlying types, since |
||
|
||
return { found: true, deposits: enrichedDeposits }; | ||
} | ||
|
||
/** | ||
* Retrieves the fill status for a given relay data from the SVM chain. | ||
*/ | ||
|
Uh oh!
There was an error while loading. Please reload this page.