improve(SpokePool): _depositV3 interprets exclusivityParameter as an offset, or a timestamp
#670
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -144,6 +144,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 * | ||
| ****************************************/ | ||
|
|
@@ -429,9 +433,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 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 | ||
|
|
@@ -470,9 +473,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 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. | ||
| */ | ||
|
|
@@ -487,62 +498,23 @@ abstract contract SpokePool is | |
| address exclusiveRelayer, | ||
| uint32 quoteTimestamp, | ||
| uint32 fillDeadline, | ||
| uint32 exclusivityParameter, | ||
| bytes calldata message | ||
| ) public payable override nonReentrant unpausedDeposits { | ||
| _depositV3( | ||
| depositor, | ||
| recipient, | ||
| inputToken, | ||
| outputToken, | ||
| inputAmount, | ||
| outputAmount, | ||
| destinationChainId, | ||
| exclusiveRelayer, | ||
| // Increment count of deposits so that deposit ID for this spoke pool is unique. | ||
| numberOfDeposits++, | ||
| quoteTimestamp, | ||
| fillDeadline, | ||
| exclusivityParameter, | ||
| message | ||
| ); | ||
| } | ||
|
|
@@ -763,7 +735,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. 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 | ||
|
|
@@ -778,7 +752,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.exclusivityDeadline, uint32(getCurrentTime())) && | ||
| relayData.exclusiveRelayer != msg.sender | ||
| ) { | ||
| revert NotExclusiveRelayer(); | ||
|
|
@@ -824,7 +798,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.exclusivityDeadline, uint32(getCurrentTime())) && | ||
| relayData.exclusiveRelayer != msg.sender | ||
| ) { | ||
| revert NotExclusiveRelayer(); | ||
|
|
@@ -873,7 +847,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.exclusivityDeadline, currentTime)) { | ||
| revert NoSlowFillsInExclusivityWindow(); | ||
| } | ||
| if (relayData.fillDeadline < currentTime) revert ExpiredFillDeadline(); | ||
|
|
@@ -1048,6 +1022,99 @@ abstract contract SpokePool is | |
| * INTERNAL FUNCTIONS * | ||
| **************************************/ | ||
|
|
||
| function _depositV3( | ||
| address depositor, | ||
| address recipient, | ||
| address inputToken, | ||
| address outputToken, | ||
| uint256 inputAmount, | ||
| uint256 outputAmount, | ||
| uint256 destinationChainId, | ||
| address exclusiveRelayer, | ||
| uint32 depositId, | ||
| uint32 quoteTimestamp, | ||
| uint32 fillDeadline, | ||
| 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 | ||
| // which is pulled from the relayer at fill time and passed through this contract atomically to the recipient. | ||
| if (!enabledDepositRoutes[inputToken][destinationChainId]) revert DisabledRoute(); | ||
|
|
||
| // Require that quoteTimestamp has a maximum age so that depositors pay an LP fee based on recent HubPool usage. | ||
| // It is assumed that cross-chain timestamps are normally loosely in-sync, but clock drift can occur. If the | ||
| // SpokePool time stalls or lags significantly, it is still possible to make deposits by setting quoteTimestamp | ||
| // within the configured buffer. The owner should pause deposits/fills if this is undesirable. | ||
| // This will underflow if quoteTimestamp is more than depositQuoteTimeBuffer; | ||
| // this is safe but will throw an unintuitive error. | ||
|
|
||
| // slither-disable-next-line timestamp | ||
| uint256 currentTime = getCurrentTime(); | ||
| if (currentTime - quoteTimestamp > depositQuoteTimeBuffer) revert InvalidQuoteTimestamp(); | ||
|
|
||
| // fillDeadline is relative to the destination chain. | ||
| // Don’t allow fillDeadline to be more than several bundles into the future. | ||
| // This limits the maximum required lookback for dataworker and relayer instances. | ||
| // Also, don't allow fillDeadline to be in the past. This poses a potential UX issue if the destination | ||
| // chain time keeping and this chain's time keeping are out of sync but is not really a practical hurdle | ||
| // unless they are significantly out of sync or the depositor is setting very short fill deadlines. This latter | ||
| // 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
+1072
to
+1074
There was a problem hiding this comment. 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. 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.
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? There was a problem hiding this comment. I also want to backtrack this statement a bit as I think fixed timestamps can be useful when building an exclusive quoting API as I describe here
There was a problem hiding this comment.
I'm probably missing something, but I'm struggling to see how this is useful vs. an offset for short-duration exclusivity. Considering the three chains where we currently see reorgs (mainnet, Scroll, Polygon): For mainnet, in my view 15 - 30 seconds in an absolute timestamp isn't enough because confirmation time is probably 6 - 12 seconds, and any relayer probably wants more than 1 deposit confirmation on top before filling. In that case I think a relative offset of 30 seconds is probably OK, since the relayer has 2 confirmation blocks, after which they can be reasonably confident that they won't see a subsequent re-org (based on historical re-org depths, at least). For Scroll, where we are now seeing reorgs at ~2 blocks, a relative offset of 15 seconds is probably fine and you don't need to revert to a fixed timestamp. I'm not sure how I see it on Polygon...their re-orgs are still fairly frequent and I see 2 at 13 & 14 blocks respectively in the last week, which amounts to ~30 seconds, and they can run much more deeply (20 - 30 blocks). So I'm unsure whether we'd be looking to apply a 30 second exclusivity there anyway. The example of 30 seconds might just be an off-the-cuff suggestion so maybe there are other examples that fit better. But I think my gut feel could be summarised as: Fixed timestamps are mostly useful over the short term to minimise invalid fills in cases where a re-org occurs without deposit reordering, assuming the initial deposit submission & confirmation delay is very short. In order to account for those delays, we may need to pad exclusivity to a duration where the origin chain re-org risk can instead be managed by the relayer by waiting for their standard finality constraints to elapse, and then filling before the residual exclusivity expires. This is the relayer's responsibility, and I think the relayers that have been burned by origin chain re-orgs have actually been discarding their finality constraints in order to prioritise filling ASAP, which IMO is long-term unsustainable because they will get burned by other re-org issues (deposit renumbering or inputAmount changes due to pre-deposit swaps). Another challenge with offering fixed timestamps is that it will play poorly with suggested-fees caching. We currently cache the fees for 10s, so a depositor might actually be approving a quote that's already 10 seconds old. We can drop this of course (and we may not see any performance impact from that, tbf), but we may alternatively need to look at other mitigations if not. There was a problem hiding this comment. I'm not sure I understand your suggestion here. So I'll propose how I think we can use fixed timestamps to support exclusivity on deposits from origin chains that re-org:
What's wrong with this strawman implementation? Fillers are incentivized via off-chain mechanisms to fill quickly, and are not penalized if the depositor takes too long to mine
This is a good critique. We may have to turn off caching for quotes on origin chains that can-reorg or turn the caching time down from 10 seconds to 2 seconds, for example There was a problem hiding this comment.
If exclusivityDeadline is set to quote time + 60 seconds as an absolute timestamp and the relayer doesn't turn up then there's an somewhat-nasty delay that the depositor suffers. Alternatively, I think you can actually get something close to 30 seconds with a relative timestamp because this implies 2 deposit confirmations + x blocks to fill on an L2 (2 - 3 blocks on OP stack chains, more on Arbitrum, 1 - 2 blocks on Linea or Scroll). So my thinking is that the utility of an absolute exclusivityDeadline is greatest when it is closer to the quote time, but this means that the uncertain time for deposit submission + confirmation "eats" most of it and reduces its utility. To me, the fact that relayers were impacted by this whilst skipping their usual finality constraints seems significant. The fact that relative timestamps also broke deterministic deposit IDs was also significant, though that is addressed by permitting the depositor to specify exclusivityDeadline 0 and have that passed through to the resulting event. (also to clarify, this might technically be a redundant point to debate because there's no obligation to use absolute timestamps. The deposit originator can choose whichever. The only impact of supporting them is a minor gas increase implied by having to check for them) |
||
| uint32 exclusivityDeadline = exclusivityParameter; | ||
| if (exclusivityDeadline > 0) { | ||
| if (exclusivityDeadline <= MAX_EXCLUSIVITY_PERIOD_SECONDS) { | ||
| exclusivityDeadline += uint32(currentTime); | ||
| } | ||
|
|
||
| // 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(); | ||
nicholaspai marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| // 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. | ||
| if (inputToken == address(wrappedNativeToken) && msg.value > 0) { | ||
| if (msg.value != inputAmount) revert MsgValueDoesNotMatchInputAmount(); | ||
| wrappedNativeToken.deposit{ value: msg.value }(); | ||
| // Else, it is a normal ERC20. In this case pull the token from the caller as per normal. | ||
| // Note: this includes the case where the L2 caller has WETH (already wrapped ETH) and wants to bridge them. | ||
| // In this case the msg.value will be set to 0, indicating a "normal" ERC20 bridging action. | ||
| } else { | ||
| // msg.value should be 0 if input token isn't the wrapped native token. | ||
| if (msg.value != 0) revert MsgValueDoesNotMatchInputAmount(); | ||
| IERC20Upgradeable(inputToken).safeTransferFrom(msg.sender, address(this), inputAmount); | ||
| } | ||
|
|
||
| emit V3FundsDeposited( | ||
| inputToken, | ||
| outputToken, | ||
| inputAmount, | ||
| outputAmount, | ||
| destinationChainId, | ||
| depositId, | ||
| quoteTimestamp, | ||
| fillDeadline, | ||
| exclusivityDeadline, | ||
| depositor, | ||
| recipient, | ||
| exclusiveRelayer, | ||
| message | ||
| ); | ||
| } | ||
|
|
||
| function _deposit( | ||
| address depositor, | ||
| address recipient, | ||
|
|
@@ -1359,13 +1426,9 @@ abstract contract SpokePool is | |
| } | ||
| } | ||
|
|
||
| // Determine whether the exclusivityDeadline implies active exclusivity. | ||
| function _fillIsExclusive(uint32 exclusivityDeadline, uint32 currentTime) internal pure returns (bool) { | ||
nicholaspai marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 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
exclusivityParameteris 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.quoteTimethat the quote was givendepositTimethat the deposit was minedfillTimethat the fill was minedfillTime - depositTime <= longestFillPeriodif thedepositTime - quoteTime <= depositBufferPeriodIMO 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
V3RelayDatastruct.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
quoteTimeordepositBufferPeriod.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
exclusivityDeadlineout of therelayDatadefinition?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.