Skip to content

improve(SpokePoolClient): Negate non-EVM SpeedUp recipients #1091

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

improve(SpokePoolClient): Negate non-EVM SpeedUp recipients #1091

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b33091d
improve(SpokePoolClient): Negate non-EVM SpeedUp recipients
pxrl Jun 19, 2025
6a6a8a3
Fix test
pxrl Jun 19, 2025
2c4de14
reflow
pxrl Jun 19, 2025
67bd40d
Merge remote-tracking branch 'origin/master' into pxrl/addressSpeedUp
pxrl Jun 26, 2025
07f480f
Require depositors to match
pxrl Jun 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions src/clients/SpokePoolClient/SpokePoolClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -572,12 +572,13 @@ export abstract class SpokePoolClient extends BaseAbstractClient {
assign(this.speedUps, [speedUp.depositor, speedUp.depositId.toString()], [speedUp]);

// Find deposit hash matching this speed up event and update the deposit data associated with the hash,
// if the hash+data exists.
// if the hash+data exists. nb. Relying on depositId alone can produce collisions on deterministic deposit IDs.
const deposit = this.getDeposit(speedUp.depositId);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nb. This isn't robust to deterministic deposit IDs. That's wrong, but speed ups are so rarely used that it's a low-priority fix.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what would happen if we try to getDeposit using a deterministic ID? Would it hang? We should probably skip it right?

Copy link
Contributor Author

@pxrl pxrl Jun 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should work fine to use it with a deterministic deposit ID in isolation; the challenge is if there are duplicate depositIds - in that case we may attach the speedup to the wrong one, and that would cause the eventual fill to fail.

I'll circle back to this PR and add some additional conditions to ensure we only attach a matching speedup to the deposit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented: 07f480f

Requiring depositor to match should at least prevent incorrectly attaching the wrong speedup to a deposit.


// We can assume all deposits in this lookback window are loaded in-memory already so if the depositHash
// is not mapped to a deposit, then we can throw away the speedup as it can't be applied to anything.
if (isDefined(deposit)) {
// SpeedUp requests are only supported EVM -> EVM.
if (isDefined(deposit) && chainIsEvm(deposit.destinationChainId) && deposit.depositor === speedUp.depositor) {
// We can assume all deposits in this lookback window are loaded in-memory already so if the depositHash
// is not mapped to a deposit, then we can throw away the speedup as it can't be applied to anything.
const eventKey = getRelayEventKey(deposit);
this.depositHashes[eventKey] = this.appendMaxSpeedUpSignatureToDeposit(deposit);
}
Expand Down