-
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?
Changes from all commits
33c6d8e
108019e
2e3d671
cf3e963
789299c
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 |
---|---|---|
|
@@ -143,6 +143,10 @@ abstract contract SpokePool is | |
// token for the input token. By using this magic value, off-chain validators do not have to keep | ||
// this event in their lookback window when querying for expired deposts. | ||
uint32 public constant INFINITE_FILL_DEADLINE = type(uint32).max; | ||
|
||
// 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; | ||
/**************************************** | ||
* EVENTS * | ||
****************************************/ | ||
|
@@ -490,9 +494,8 @@ abstract contract SpokePool is | |
/** | ||
* @notice Previously, this function allowed the caller to specify the exclusivityDeadline, otherwise known as the | ||
* as exact timestamp on the destination chain before which only the exclusiveRelayer could fill the deposit. Now, | ||
* the caller is expected to pass in an exclusivityPeriod which is the number of seconds to be added to the | ||
* block.timestamp to produce the exclusivityDeadline. This allows the caller to ignore any latency associated | ||
* with this transaction being mined and propagating this transaction to the miner. | ||
* the caller is expected to pass in a number that will be interpreted either as an offset or a fixed | ||
* timestamp depending on its value. | ||
* @notice Request to bridge input token cross chain to a destination chain and receive a specified amount | ||
* of output tokens. The fee paid to relayers and the system should be captured in the spread between output | ||
* amount and input amount when adjusted to be denominated in the input token. A relayer on the destination | ||
|
@@ -531,9 +534,17 @@ abstract contract SpokePool is | |
* @param fillDeadline The deadline for the relayer to fill the deposit. After this destination chain timestamp, | ||
* the fill will revert on the destination chain. Must be set between [currentTime, currentTime + fillDeadlineBuffer] | ||
* where currentTime is block.timestamp on this chain or this transaction will revert. | ||
* @param exclusivityPeriod Added to the current time to set the exclusive relayer deadline, | ||
* which is the deadline for the exclusiveRelayer to fill the deposit. After this destination chain timestamp, | ||
* anyone can fill the deposit. | ||
* @param exclusivityParameter This value is used to set the exclusivity deadline timestamp in the emitted deposit | ||
* event. Before this destinationchain timestamp, only the exclusiveRelayer (if set to a non-zero address), | ||
* can fill this deposit. There are three ways to use this parameter: | ||
* 1. NO EXCLUSIVITY: If this value is set to 0, then a timestamp of 0 will be emitted, | ||
* meaning that there is no exclusivity period. | ||
* 2. OFFSET: If this value is less than MAX_EXCLUSIVITY_PERIOD_SECONDS, then add this value to | ||
* the block.timestamp to derive the exclusive relayer deadline. Note that using the parameter in this way | ||
* will expose the filler of the deposit to the risk that the block.timestamp of this event gets changed | ||
* due to a chain-reorg, which would also change the exclusivity timestamp. | ||
* 3. TIMESTAMP: Otherwise, set this value as the exclusivity deadline timestamp. | ||
* which is the deadline for the exclusiveRelayer to fill the deposit. | ||
* @param message The message to send to the recipient on the destination chain if the recipient is a contract. | ||
* If the message is not empty, the recipient contract must implement handleV3AcrossMessage() or the fill will revert. | ||
*/ | ||
|
@@ -548,7 +559,7 @@ abstract contract SpokePool is | |
address exclusiveRelayer, | ||
uint32 quoteTimestamp, | ||
uint32 fillDeadline, | ||
uint32 exclusivityPeriod, | ||
uint32 exclusivityParameter, | ||
bytes calldata message | ||
) public payable override nonReentrant unpausedDeposits { | ||
_depositV3( | ||
|
@@ -566,7 +577,7 @@ abstract contract SpokePool is | |
numberOfDeposits++, | ||
quoteTimestamp, | ||
fillDeadline, | ||
uint32(getCurrentTime()) + exclusivityPeriod, | ||
exclusivityParameter, | ||
message | ||
); | ||
} | ||
|
@@ -600,7 +611,7 @@ abstract contract SpokePool is | |
* @param exclusiveRelayer See identically named parameter in depositV3() comments. | ||
* @param quoteTimestamp See identically named parameter in depositV3() comments. | ||
* @param fillDeadline See identically named parameter in depositV3() comments. | ||
* @param exclusivityPeriod See identically named parameter in depositV3() comments. | ||
* @param exclusivityParameter See identically named parameter in depositV3() comments. | ||
* @param message See identically named parameter in depositV3() comments. | ||
*/ | ||
function unsafeDepositV3( | ||
|
@@ -615,7 +626,7 @@ abstract contract SpokePool is | |
uint256 depositNonce, | ||
uint32 quoteTimestamp, | ||
uint32 fillDeadline, | ||
uint32 exclusivityPeriod, | ||
uint32 exclusivityParameter, | ||
bytes calldata message | ||
) public payable nonReentrant unpausedDeposits { | ||
// @dev Create the uint256 deposit ID by concatenating the msg.sender and depositor address with the inputted | ||
|
@@ -636,7 +647,7 @@ abstract contract SpokePool is | |
depositId, | ||
quoteTimestamp, | ||
fillDeadline, | ||
uint32(getCurrentTime()) + exclusivityPeriod, | ||
exclusivityParameter, | ||
message | ||
); | ||
} | ||
|
@@ -857,7 +868,9 @@ abstract contract SpokePool is | |
* - fillDeadline The deadline for the caller to fill the deposit. After this timestamp, | ||
* the fill will revert on the destination chain. | ||
* - exclusivityDeadline: The deadline for the exclusive relayer to fill the deposit. After this | ||
* timestamp, anyone can fill this deposit. | ||
* timestamp, anyone can fill this deposit. Note that if this value was set in depositV3 by adding an offset | ||
* to the deposit's block.timestamp, there is re-org risk for the caller of this method because the event's | ||
* block.timestamp can change. Read the comments in `depositV3` about the `exclusivityParameter` for more details. | ||
* - message The message to send to the recipient if the recipient is a contract that implements a | ||
* handleV3AcrossMessage() public function | ||
* @param repaymentChainId Chain of SpokePool where relayer wants to be refunded after the challenge window has | ||
|
@@ -872,7 +885,7 @@ abstract contract SpokePool is | |
// Exclusivity deadline is inclusive and is the latest timestamp that the exclusive relayer has sole right | ||
// to fill the relay. | ||
if ( | ||
_fillIsExclusive(relayData.exclusiveRelayer, relayData.exclusivityDeadline, uint32(getCurrentTime())) && | ||
_fillIsExclusive(relayData.exclusivityDeadline, uint32(getCurrentTime())) && | ||
relayData.exclusiveRelayer != msg.sender | ||
) { | ||
revert NotExclusiveRelayer(); | ||
|
@@ -918,7 +931,7 @@ abstract contract SpokePool is | |
// Exclusivity deadline is inclusive and is the latest timestamp that the exclusive relayer has sole right | ||
// to fill the relay. | ||
if ( | ||
_fillIsExclusive(relayData.exclusiveRelayer, relayData.exclusivityDeadline, uint32(getCurrentTime())) && | ||
_fillIsExclusive(relayData.exclusivityDeadline, uint32(getCurrentTime())) && | ||
relayData.exclusiveRelayer != msg.sender | ||
) { | ||
revert NotExclusiveRelayer(); | ||
|
@@ -967,7 +980,7 @@ abstract contract SpokePool is | |
// fast fill within this deadline. Moreover, the depositor should expect to get *fast* filled within | ||
// this deadline, not slow filled. As a simplifying assumption, we will not allow slow fills to be requested | ||
// during this exclusivity period. | ||
if (_fillIsExclusive(relayData.exclusiveRelayer, relayData.exclusivityDeadline, currentTime)) { | ||
if (_fillIsExclusive(relayData.exclusivityDeadline, currentTime)) { | ||
revert NoSlowFillsInExclusivityWindow(); | ||
} | ||
if (relayData.fillDeadline < currentTime) revert ExpiredFillDeadline(); | ||
|
@@ -1165,7 +1178,7 @@ abstract contract SpokePool is | |
uint256 depositId, | ||
uint32 quoteTimestamp, | ||
uint32 fillDeadline, | ||
uint32 exclusivityDeadline, | ||
uint32 exclusivityParameter, | ||
bytes calldata message | ||
) internal { | ||
// Check that deposit route is enabled for the input token. There are no checks required for the output token | ||
|
@@ -1192,6 +1205,32 @@ abstract contract SpokePool is | |
// situation won't be a problem for honest users. | ||
if (fillDeadline < currentTime || fillDeadline > currentTime + fillDeadlineBuffer) revert InvalidFillDeadline(); | ||
|
||
// There are three cases for setting the exclusivity deadline using the exclusivity parameter: | ||
// 1. If this parameter is 0, then there is no exclusivity period and emit 0 for the deadline. This | ||
// means that fillers of this deposit do not have to worry about the block.timestamp of this event changing | ||
// due to re-orgs when filling this deposit. | ||
// 2. If the exclusivity parameter is less than or equal to MAX_EXCLUSIVITY_PERIOD_SECONDS, then the exclusivity | ||
// deadline is set to the block.timestamp of this event plus the exclusivity parameter. This means that the | ||
// filler of this deposit assumes re-org risk when filling this deposit because the block.timestamp of this | ||
// event affects the exclusivity deadline. | ||
// 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. | ||
Comment on lines
+1216
to
+1218
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. 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
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? |
||
uint32 exclusivityDeadline; | ||
if (exclusivityParameter == 0) { | ||
exclusivityDeadline = 0; | ||
} else { | ||
if (exclusivityParameter <= MAX_EXCLUSIVITY_PERIOD_SECONDS) { | ||
exclusivityDeadline = uint32(currentTime) + exclusivityParameter; | ||
} else { | ||
exclusivityDeadline = exclusivityParameter; | ||
} | ||
|
||
// As a safety measure, prevent caller from inadvertently locking funds during exclusivity period | ||
// by forcing them to specify an exclusive relayer. | ||
if (exclusiveRelayer == address(0)) revert InvalidExclusiveRelayer(); | ||
} | ||
|
||
// If the address of the origin token is a wrappedNativeToken contract and there is a msg.value with the | ||
// transaction then the user is sending the native token. In this case, the native token should be | ||
// wrapped. | ||
|
@@ -1535,13 +1574,9 @@ abstract contract SpokePool is | |
} | ||
} | ||
|
||
// Determine whether the combination of exlcusiveRelayer and exclusivityDeadline implies active exclusivity. | ||
function _fillIsExclusive( | ||
address exclusiveRelayer, | ||
uint32 exclusivityDeadline, | ||
uint32 currentTime | ||
) 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 commentThe 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 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 |
||
return exclusivityDeadline >= currentTime; | ||
} | ||
|
||
// Implementing contract needs to override this to ensure that only the appropriate cross chain admin can execute | ||
|
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:
depositBufferPeriod
. The filler gets this amount of protection against deposits taking longer to mine.longestFillPeriod
. The filler must fill within this time of the deposit being mined.quoteTime
that the quote was givendepositTime
that the deposit was minedfillTime
that the fill was minedfillTime - depositTime <= longestFillPeriod
if thedepositTime - quoteTime <= depositBufferPeriod
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
ordepositBufferPeriod
.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 therelayData
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.