Non-linear Block-based Orders #269
Conversation
src/lib/NonLinearDutchOrderLib.sol
Outdated
| // The tokens that must be received to satisfy the order | ||
| NonLinearDutchOutput[] baseOutputs; | ||
| // signed over by the cosigner | ||
| CosignerData cosignerData; |
There was a problem hiding this comment.
since these are purely relative, the cosigner now has full power to set the start amount to whatever they want? some thoughts here:
- this is a lot more power than cosigner has had in the past. Maybe we want to consider setting a min/max value?
- potentially cosigned start/end amounts could be so high/low to cause under/overflow after adjustment - probably want to make sure we cover that case
There was a problem hiding this comment.
The cosigner can only update the startAmount (not the relative amounts) and only if it's better than what the swapper signed (see here). The curve and base amounts are set by the swapper. Because of this, I don't think the cosigner has more trust than it did in the past. LMK if I'm missing something.
If the cosigner is malicious and attempts to cause overflow, the tx will revert (as it's currently implemented). Underflow isn't possible since we're subtracting an int256 from a uint256. In the case of overflow we could either:
- Do nothing and have the swapper deal with the failed tx (eg. use another cosigner).
- Detect the overflow in the reactor and fallback to using the user provided input. This will be more gas intensive since we'd need to loop through the curve and check each point.
Given this could only occur with a malicious cosigner and the worst case is that the tx reverts, I think it's safe to just let the tx revert.
src/lib/NonLinearDutchDecayLib.sol
Outdated
| } | ||
| uint256 blockDelta = block.number - decayStartBlock; | ||
| // iterate through the points and locate the current segment | ||
| for (uint256 i = 0; i < curve.relativeAmount.length; i++) { |
There was a problem hiding this comment.
also because the max number of points on the curve is known and strictly increasing, we can optimize this via a binary search once we know if blockDelta is greater than or equal to the mid value in the curve
There was a problem hiding this comment.
I've implemented this in this commit however, I think it actually costs more gas for the length of points we're dealing with.
Linear Search
Binary Search
There was a problem hiding this comment.
Performed more extensive gas testing comparing linear with binary searching. Binary search only starts showing real benefits when we have 14 or more curve points. Opting for linear search since we will likely have less than 5 curve points defined and it's less bug prone.
|
seems you need a |
* Gas adjustment feature and tests * Update 712 structs * Final tests * Address PR feedback * Natspec * PR Feedback * edge case fix and gas improvements
src/lib/NonlinearDutchDecayLib.sol
Outdated
| Uint16Array relativeBlocks = fromUnderlying(curve.relativeBlocks); | ||
| uint16 blockDelta = uint16(block.number - decayStartBlock); | ||
| int256 curveDelta; | ||
| // Special case for when we need to use the decayStartBlock (0) |
There was a problem hiding this comment.
I think this comment could use a little more color, taking me some effort to figure out what's happening here
So decayStartBlock is already past, but the first block delta hasn't been hit yet? So you're anchoring the left end on block 0? I'm not sure I see the reason even for linear decay there, since it's just going to be sooo close to startAmount + relativeAmounts[0] right?
There was a problem hiding this comment.
edit: ohh it's blockDelta of 0, not block 0 right?
There was a problem hiding this comment.
this is one case where maybe named parameters for linearDecay would help
There was a problem hiding this comment.
Possible to just contain this case within locateCurvePosition?
There was a problem hiding this comment.
Just refactored it.. lmk what you thnk!
codyborn
dismissed
zhongeric’s stale review
Addressed review comments. Eric is out this week so can't respond.
Some notable differences from Dutchv2 Orders:
uint256 endAmount, input and output use aNonLinearDecay curvestruct, which defines the relative points on the decay curve. The points are relative so that the cosigner only needs to override thestartAmountand the user’s signed curve can remain unchanged.uint16to store the relative block numbers. With 16 bits, we can define curves with points far into the future (11 days on Arbitrum and 1.5 years on Mainnet). If we were to use 8 bits, we could only define curves with 64 seconds into the future on Arbitrum.NonLinearDutchInputnow contains amaxAmountproperty to hold the max point on the input curve. Used for constructing theInputTokenobject for Permit2.