chore: Report invalidFills for determenistic deposits#1457
chore: Report invalidFills for determenistic deposits#1457dijanin-brat wants to merge 7 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3bbd37f30
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (getCurrentTime() - fillTimestamp < unsafeDepositGracePeriodSec) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
When an unsafe/deterministic fill is older than the grace period and the origin SpokePoolClient has not already indexed a matching deposit, this branch falls through to spokePoolClients[fill.originChainId]?.findDeposit(fill.depositId). The historical lookup path explicitly rejects unsafe IDs (src/arch/evm/SpokeUtils.ts:140-143 and src/arch/svm/SpokeUtils.ts:263-266 throw for isUnsafeDepositId), so findInvalidFills rejects instead of reporting an InvalidFill; the new test masks this by mocking findDeposit to return not found for all IDs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Codex Review: Didn't find any major issues. 🎉
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (unsafeDeposit) { | ||
| const fillTimestamp = await spokePoolClient.getTimestampForBlock(fill.blockNumber); | ||
| if (getCurrentTime() - fillTimestamp < unsafeDepositGracePeriodSec) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
It's relatively expensive to do this lookup if we already have the deposit - wdyt about deferring the timestamp resolution until after we've verified that we don't actually have a deposit for this fill?
| // deposit to be indexed before treating the fill as invalid. | ||
| if (unsafeDeposit) { | ||
| const fillTimestamp = await spokePoolClient.getTimestampForBlock(fill.blockNumber); | ||
| if (getCurrentTime() - fillTimestamp < unsafeDepositGracePeriodSec) { |
There was a problem hiding this comment.
Specifically on timestamp resolution itself, I'm also wondering if there's something cheaper that we can do - i.e. could we reference the fillDeadline instead? I'm tbf not sure if this works in all cases. An alternative could be to infer the age of the fill based an interpolation of the delta between spokePoolClient.latestHeightSearched and fill.blockNumber, where we multiply by the average block time (we should have that on-hand).
There was a problem hiding this comment.
I don't think its smart to use fillDeadline for this.
I think its much better to calculate the timestamp as you have described
a66093f
|
@codex re-review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a66093f5ab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // deposits already indexed in the origin SpokePoolClient's memory. | ||
| let deposit; | ||
| if (unsafeDeposit) { | ||
| deposit = originClient?.getDeposit(fill.depositId); |
There was a problem hiding this comment.
For deterministic deposits, the same unsafe depositId can be reused for different relay data (the codebase already notes that relying on depositId alone can collide for deterministic IDs). This lookup returns only the first indexed deposit with that ID, so if two unsafe deposits share an ID and the fill corresponds to a later one, validateFillForDeposit will compare against the wrong deposit and report a valid fill as invalid. Please search the indexed origin deposits for a matching relay/deposit rather than a single getDeposit(depositId) result.
Useful? React with 👍 / 👎.
2 participants
No description provided.