-
Notifications
You must be signed in to change notification settings - Fork 86
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
Gas Adjustment Feature #273
Conversation
src/reactors/V3DutchOrderReactor.sol
Outdated
int256 gasDeltaGwei = block.basefee.sub(order.gasSnapshot); | ||
|
||
// Gas increase should increase input | ||
int256 inputDelta = int256(order.baseInput.adjustmentPerGweiBasefee) * gasDeltaGwei / 1 gwei; |
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.
is gwei too large of a denominator? I wonder if wei or some percentage of a gwei would be better, then we can move this into a constant
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.
mulDivDown? or does that not support signed ints
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.
@marktoda yeah mulDivDown only supports uint256.
@zhongeric I was thinking about this a lot too.. I think gwei is the easiest to reason about with today's gas prices. The int256 gives us ~1e76 of space and if we assume gas is around 1e9-1e11 we get a range of 1 wei - 1e65 for possible adjustmentPerGweiBasefee
values.
If we look at low value token like SHIB for example which has 18 decimals and is worth 0.0000000057064226 ETH, 1 gwei = 5.7064226 SHIB, so we'd want the adjustmentPerGweiBasefee = 5706422600 * 10k gas
and we'd still have 1e53 of wiggle room before overflowing.
/// @param currentPoint The current position in the decay | ||
/// @param startAmount The amount of the start of the decay | ||
/// @param endAmount The amount of the end of the decay | ||
function linearDecay( |
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 added another version of this that accepts int256 for start and end amount.
- I wanted to decay only through the relative amounts so that I can apply the bounding logic (min/max) check after the decay.
- It was too convoluted to try to support both in the same function.
Note that this linearDecay performs mulDivDown
in both positive and negative cases as it felt more intuitive. Happy to discuss!
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.
can the uint one call this one w/ casted params?
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.
Yup! I was trying to do the other way around, but as long as we're okay with losing 1 bit of precision, it's cleaner.
OrderInfoLib.ORDER_INFO_TYPE, | ||
DutchOrderLib.TOKEN_PERMISSIONS_TYPE | ||
DutchOrderLib.TOKEN_PERMISSIONS_TYPE, | ||
V3_DUTCH_INPUT_TYPE, |
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.
Some of these are fixes from this PR
#269
src/lib/DutchDecayLib.sol
Outdated
return endAmount; | ||
} | ||
unchecked { | ||
uint256 elapsed = currentPoint - startPoint; |
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 need to check for startPoint > currentPoint to avoid overflow here?
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.
Good question. It's not possible with the current code since the caller passes in either 0 or relativeBlocks.getElement(prev)
for the startPoint
where relativeBlocks.getElement(prev)
is guaranteed to be less than the currentPoint
.
Similarly it's not possible for the startPoint
to be greater than or equal to the endPoint
. It could be equal if (currentPoint > endPoint) but we check that above. Otherwise, the locateCurvePosition()
call ensures that it halts the search as soon as it finds a block that is greater than or equal to the blockDelta
, meaning every value that came before it would be less than the blockDelta
(and transitively less than the block it finds).
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 IS possible for this int256 cast to overflow if startAmount
is max int256 and endAmount
is negative:
int256(0 - uint256(startAmount - endAmount).mulDivDown(elapsed, duration)
Going to remove the unchecked
.
/// @param currentPoint The current position in the decay | ||
/// @param startAmount The amount of the start of the decay | ||
/// @param endAmount The amount of the end of the decay | ||
function linearDecay( |
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.
can the uint one call this one w/ casted params?
src/reactors/V3DutchOrderReactor.sol
Outdated
int256 gasDeltaGwei = block.basefee.sub(order.gasSnapshot); | ||
|
||
// Gas increase should increase input | ||
int256 inputDelta = int256(order.baseInput.adjustmentPerGweiBasefee) * gasDeltaGwei / 1 gwei; |
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.
mulDivDown? or does that not support signed ints
src/lib/V3DutchOrderLib.sol
Outdated
output.recipient | ||
output.recipient, | ||
output.minAmount, | ||
output.adjustmentPerGweiBasefee |
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.
is gwei
enough granularity? think even partial gwei changes in basefee may be impactful
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.
Yeah, it's denominated in gwei but since the division by 1 gwei happens after the multiplication, we can take advantage of wei-level changes in the gas fee.
int256 inputDelta = int256(order.baseInput.adjustmentPerGweiBaseFee) * gasDeltaGwei / 1 gwei
See this test for example:
UniswapX/test/reactors/V3DutchOrderReactor.t.sol
Line 1127 in c0d6998
assertEq(resolvedOrder.input.amount, 1 ether - 0.5 gwei); |
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.
looks good with some comments!
@@ -74,27 +74,26 @@ contract DutchDecayLibTest is Test { | |||
assertEq(DutchDecayLib.decay(2 ether, 1 ether, 100, 200), 1 ether); | |||
} | |||
|
|||
function testDutchDecayBounded(uint256 startAmount, uint256 endAmount, uint256 decayStartTime, uint256 decayEndTime) | |||
function testDutchDecayBounded(int256 startAmount, int256 endAmount, uint256 decayStartTime, uint256 decayEndTime) |
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.
why change types to int256 to cast to uint256 in the test?
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 helps bound the values to a range to prevent overflow.. I tried vm.assume(startAmount < 2* 255 - 1)
but forge testing would fail saying it hit the max number of failing input.
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.
Ah I see, and bound would reject too many probably
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.
Niceee!!
* WIP * Builds successfully * First set of tests * Unit tests + array packing * More tests * Moar tests * Remove yarn.lock * forge fmt * Custom types * Missed file * Improve readibility and add gas test * Binary search * Linear search + gas tests * Rename to V3 * Reactor tests pt1, refactor tests, fmt * More reactor tests, address PR feedback * Forge fmt * More tests * Fix old exclusivity test * Consolidate decay logic * Address PR feedback * PR feedback * Gas Adjustment Feature (#273) * Gas adjustment feature and tests * Update 712 structs * Final tests * Address PR feedback * Natspec * PR Feedback * edge case fix and gas improvements * Simplify NonlinearDecay logic
Adjusts the auction start price based on the change in
block.basefee
from the time the swapper signed the order to the time of execution. The adjustment can be in favor of the swapper (unlimited) or against the swapper (bounded by slippage).The goal of this mainnet-focused feature is to:
To know how much to adjust the input/output token by, I introduce a new field
adjustmentPerGweiBasefee
which is the amount of token to change per gwei of change to the basefee. As @0age mentioned on the call, it should be calculated by (total gas of swap * token/eth).