-
Notifications
You must be signed in to change notification settings - Fork 50
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
improve(SpokePool): _depositV3 interprets exclusivityParameter
as an offset, or a timestamp
#670
base: mrice32/deterministic-new
Are you sure you want to change the base?
Conversation
…, an offset, or a timestamp There should be a way for the deposit transaction to remove chain re-org risk affecting the block.timestamp by allowing the caller to set a fixed `exclusivityDeadline` value. This supports the existing behavior where the `exclusivityDeadline` is always emitted as its passed in. The new behavior is that if the `exclusivityParameter`, which replaces the `exclusivityDeadlineOffset` parameter, is 0 or greater than 1 year in seconds, then the `exclusivityDeadline` is equal to this parameter. Otherwise, its interpreted by `_depositV3()` as an offset. The offset would be useful in cases where the origin chain will not re-org, for example.
// One year in seconds. If `exclusivityParameter` is set to a value less than this, then the emitted | ||
// exclusivityDeadline in a deposit event will be set to the current time plus this value. | ||
uint32 public constant MAX_EXCLUSIVITY_PERIOD_SECONDS = 31_536_000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we instead infer the upper limit for a relative exclusivity period based on the fillDeadlineBuffer? It otherwise doesn't make sense to permit relative exclusivity > fillDeadlineBuffer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with this since no relayer can make a fill on an expired deposit regardless of exclusivity.
On the other hand - having a defined upper limit for whether the exclusivityParameter
is an offset or timestamp is convenient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that seems like it would add complexity where we don't need it. If the caller sets the exclusivity period larger than fill deadline, then the full period is exclusive. Is this a bug? Not really IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's OK to permit a larger relative exclusivityDeadline, I just don't know of any use cases for a fixed exclusivityDeadline - that's generally unreliable due to the unknown transaction submission & confirmation delay, and the logic to derive whether it's 0, relative or fixed is relatively more complicated as a result (also RE #670 (comment)).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think there are ways to build an exclusive quoting API with fixed timestamps:
- API gives depositors fixed timestamps to set. Let's say this is some time 15 seconds from now.
- Depositors form deposit and then sign over message and hand off to filler to submit on origin chain
- API rewards the designated exclusive filler if they fill on the destination chain within 5 seconds of the deposit time
- API penalizes fillers who do not fill in time unless the deposit took too long to be mined on chain. In this example let's say the deposit gets 10 seconds to be brought on chain. This means the filler gets 5 seconds of exclusivity in their worst case.
- Therefore the API needs to parameterize:
- A buffer period for the deposit to be mined, in this example it is 10 seconds,
depositBufferPeriod
. The filler gets this amount of protection against deposits taking longer to mine. - A target fill time, in this example it is 5 seconds,
longestFillPeriod
. The filler must fill within this time of the deposit being mined.
- A buffer period for the deposit to be mined, in this example it is 10 seconds,
- And the API must keep track of:
- The time
quoteTime
that the quote was given - The time
depositTime
that the deposit was mined - The time
fillTime
that the fill was mined
- The time
- And the API will reward fillers who:
- Have a fill response time of
fillTime - depositTime <= longestFillPeriod
if thedepositTime - quoteTime <= depositBufferPeriod
- Have a fill response time of
IMO this use case is the only realistic way to support exclusivity without exposing fillers to block.timestamp-related re-org risk and without changing the V3RelayData
struct.
Another way to build the API is to offer relative offset exclusivity if the origin chain has very little re org risk. This protects fillers a bit more from late deposits. For these chains, the API would no longer need to keep track of quoteTime
or depositBufferPeriod
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicholaspai Have we ruled out pulling exclusivityDeadline
out of the relayData
definition?
If we did this then there's no marginal re-org risk as a result of this feature. It does permit someone else to make a fill for a relay that they didn't have exclusivity for, but the user is still OK in that context, and we can easily post-process to filter those out when evaluating relayer performance.
I guess one issue is that well- behaved relayers can still get rekt by bad actors who deliberately intend to disrupt :\
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think removing exclusivity deadline from the relay hash is off the table since it means anyone can "steal" exclusivity and it prevents the depositor from offering exclusivity. If we were open to changing the relay hash then I would instead propose adding an initiateDeadline parameter that ensures that deposits don't delay getting mined.
However changing the relay hash would be highly disruptive so I strongly want to avoid.
contracts/SpokePool.sol
Outdated
relayData.exclusivityDeadline >= getCurrentTime() && | ||
relayData.exclusiveRelayer != address(0) | ||
) { | ||
if (relayData.exclusivityDeadline >= getCurrentTime() && relayData.exclusiveRelayer != msg.sender) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK to base this on top of #649? It's approved but I just wanted to align on when it should be merged RE the ongoing audit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 3. Otherwise, interpret this parameter as a timestamp and emit it as the exclusivity deadline. This means | ||
// that the filler of this deposit will not assume re-org risk related to the block.timestamp of this | ||
// event changing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a concrete use case for this? It only seems useful for relatively long-duration (i.e. fillDeadlineBuffer > exclusivity > ~300 seconds). But that in itself seems quite bespoke since it can still be achieved with relative duration.
tldr, as the exclusivity period increases in duration, origin chain reorg risk fades to 0, so the relative timestamp actually seems like a better fit there anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO there isn't a strong reason to use a fixed timestamp over a large exclusivity offset but there are probably some cases where you want to fill pretty quickly (i.e. take on some re-org risk) and also have exclusivity over the full deposit. Why not support it since it's easy to write the code for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It only seems useful for relatively long-duration
I don't think this is necessarily true. You could imagine employing "optimistic" exclusivity where you give 15-30 seconds seconds of exclusivity and if someone takes too long to send their transaction, they just lose exclusivity.
Thoughts?
) internal pure returns (bool) { | ||
return exclusivityDeadline >= currentTime && exclusiveRelayer != address(0); | ||
// Determine whether the exclusivityDeadline implies active exclusivity. | ||
function _fillIsExclusive(uint32 exclusivityDeadline, uint32 currentTime) internal pure returns (bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pxrl wdyt of this:
I don't think there's a need to check that the exclusiveRelayer
is not the zero address. The _depositV3
function protects depositors from accidentally setting an exclusivity period but not setting an exclusive relayer.
If a filler tries to call fillV3 with an exclusivity period but no exclusive relayer, then they won't be able to fill the deposit. This is also intuitive because this type of deposit would no longer be possible.
In fact, I don't think we needed to check this exclusiveRelayer != address(0)
before, but I could be missing something
There should be a way for the deposit transaction to remove chain re-org risk affecting the block.timestamp by allowing the caller to set a fixed
exclusivityDeadline
value. This supports the existing behavior where theexclusivityDeadline
is always emitted as its passed in.The new behavior is that if the
exclusivityParameter
, which replaces theexclusivityDeadlineOffset
parameter, is 0 or greater than 1 year in seconds, then theexclusivityDeadline
is equal to this parameter. Otherwise, its interpreted by_depositV3()
as an offset. The offset would be useful in cases where the origin chain will not re-org, for example.