Migrate proportional action arguments from bps to ppm - #617
Conversation
BASIS changes from 10_000 to 1_000_000. Every action argument expressing a proportion of Settler's token balance is renamed bps->ppm (maxBps->maxPpm) and re-denominated in parts-per-million. Action selectors are unchanged. The packed proportion field widens from 2 to 3 bytes in the flash-accounting header and in each UNISWAPV4/BALANCERV3/PANCAKE_INFINITY/EKUBO/EKUBOV3 fill; per-venue _HOP_DATA_LENGTH grows by 1. The BalancerV3 wrap/unwrap flags move to bits 23/22 of the fill's proportion field and the Ekubo forwarding-extension flag to bit 23. The Permit2 balance-proportional sentinel band widens with BASIS. Pool fees carried in swapInfo (UniswapV2, Velodrome) remain denominated in bps against a hard-coded 10_000 per the pools' own convention, as does EulerSwap's 1e4 LTV scale. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jparklev
left a comment
There was a problem hiding this comment.
Nice one.
Should we mention in the README like, "the first ppm deployment on each chain is nonce N (per-chain table), earlier nonces are bps”? Not 100% whats best for integrators, but seems like a pretty large behavoiral change we wouldn't want anyone to opt into accidentally
| ( | ||
| address(fromToken()), | ||
| 10_000, | ||
| 1_000_000, |
There was a problem hiding this comment.
Should we add some sub-bp cases here to test? (eg the 30 ppm fee mentioned by Phil)
There was a problem hiding this comment.
we might, but:
- ideally I would keep the proportions of all tests to not break them.
- I don't think it brings that much value as tests should already test different proportions
| using NotesLib for NotesLib.Note[]; | ||
|
|
||
| uint256 internal constant BASIS = 10_000; | ||
| uint256 internal constant BASIS = 1_000_000; |
There was a problem hiding this comment.
thoughts on one shared like PPM_DENOM constant we import? (should be runtime bytecode neutral)
(in theory we also then wouldn't have to do like assert(BASIS == Encoder.BASIS) in BalancerV3 and pancake constructors)
There was a problem hiding this comment.
yeah, at some point we should get to have a Constants.sol for things like this and native for example
There was a problem hiding this comment.
Seems reasonable to me. Maybe add it to a Constants.sol file where we also put the ERC7528 constant?
| // implicitly advance it by 20 bytes to decode `ppm` then advance by 23 bytes | ||
|
|
||
| bps := shr(0x50, calldataload(data.offset)) | ||
| ppm := shr(0x48, calldataload(data.offset)) |
There was a problem hiding this comment.
nit: consider uint256 ppm; + ppm := shr(0xe8, calldataload(add(0x14, data.offset))).
It’s a couple bytes smaller, and then we don't need the “implicitly advance by 20 bytes” comment
(similar in erc20 branch below. could punt to a future cleanup)
There was a problem hiding this comment.
I believe the original implementation that created a dirty bps was more gas/stack efficient because it avoided an extra calldataload. But this is relatively old code that was written for an older solc that might be dumber than the current one.
It's worth testing.
| ### Breaking changes | ||
|
|
||
| * All proportional-amount action arguments are now denominated in parts-per-million (`ppm`; denominator 1_000_000) instead of basis points (`bps`; denominator 10_000) | ||
| * Affects the third-ish argument of `UNISWAPV3`, `UNISWAPV2`, `BASIC`, `VELODROME`, `MAKERPSM`, `DODOV1`, `DODOV2`, `MAVERICKV2`, `EULERSWAP`, `HANJI`, `UNISWAPV4`, `BALANCERV3`, `PANCAKE_INFINITY`, `EKUBO`, and `EKUBOV3`; the `maxBps` (now `maxPpm`) argument of `POSITIVE_SLIPPAGE`; and BridgeSettler's `BASIC` |
There was a problem hiding this comment.
"third-ish" reads as kind of odd here to me. Maybe just named like "the proportion-of-balance argument"?
There was a problem hiding this comment.
changed it to bps to be explicit about the argument affected instead of its position
🛡️ Immunefi PR ReviewsWe noticed that your project isn't set up for automatic code reviews. If you'd like this PR reviewed by the Immunefi team, you can request it manually using the link below: Once submitted, we'll take care of assigning a reviewer and follow up here. |
| using NotesLib for NotesLib.Note[]; | ||
|
|
||
| uint256 internal constant BASIS = 10_000; | ||
| uint256 internal constant BASIS = 1_000_000; |
There was a problem hiding this comment.
Seems reasonable to me. Maybe add it to a Constants.sol file where we also put the ERC7528 constant?
| using NotesLib for NotesLib.Note[]; | ||
|
|
||
| uint256 internal constant BASIS = 10_000; | ||
| uint256 internal constant BASIS = 1_000_000; |
| // implicitly advance it by 20 bytes to decode `ppm` then advance by 23 bytes | ||
|
|
||
| bps := shr(0x50, calldataload(data.offset)) | ||
| ppm := shr(0x48, calldataload(data.offset)) |
There was a problem hiding this comment.
I believe the original implementation that created a dirty bps was more gas/stack efficient because it avoided an extra calldataload. But this is relatively old code that was written for an older solc that might be dumber than the current one.
It's worth testing.
Summary
Breaking change. Every action argument that expresses a proportion of Settler's token balance is re-denominated from basis points (
BASIS = 10_000) to parts-per-million (BASIS = 1_000_000), and renamedbps→ppm(maxBps→maxPpminPOSITIVE_SLIPPAGE). This makes sub-bp proportions expressible (e.g. a 30 ppm fee) and stops small fee proportions from truncating to zero on large-notional trades.Action selectors are unchanged (parameter names are not part of the selector), so the unit is a property of the Settler deployment: encoders must key bps vs ppm on the Settler address they target.
Details
BASISbecomes1_000_000inSettlerAbstract,Encoder, andDecoder(the constructor cross-check asserts are unchanged and still hold).Encoder.encode/Decoder.initializeinFlashAccountingCommon.sol); VIP headers carry no proportion and are untouched;UNISWAPV4,BALANCERV3,PANCAKE_INFINITY,EKUBO, andEKUBOV3._HOP_DATA_LENGTHgrows by 1 per venue (52→53, 3→4, 82→83, 47→48, 47→48).0xc00000/0x400000/0x3fffff), Ekubo forwarding-extension bit 15 → 23 (0x800000/0x7fffff).~amount < BASIS) and the RFQ maker-permit asserts widen automatically with the constant.[Unreleased]CHANGELOG entry documents the full encoding change for integrators;sh/initial_description_*.mdare updated so future chain bring-ups registerppmwording (existing chains' feature descriptions are immutable one-shot IPFS metadata and are unaffected).Deliberately unchanged
Pool fees carried in the upper 16 bits of
swapInfo(UniswapV2, Velodrome, VelodromeAlt) stay denominated in bps against a hard-coded10_000: they describe the external pool's own fee convention, and 1e6 does not fit in that 16-bit field. Likewise EulerSwap's 1e4 LTV scale and MakerPSM'sGEM_basis.Testing
uint16/bytes2→uint24/bytes3, flag constructions moved to bits 23/22.FOUNDRY_PROFILE=integrationcompiles; fork tests + gas comparison to be confirmed by CI.forge build --sizespasses for every chain inchain_config.json.Gas Optimization
No hot-path changes; the arithmetic is identical (
* ppm / BASIS). Calldata grows 1 byte per fill plus 1 byte per flash-accounting action. Contract size deltas vsmaster(95184a2) on the tightest chain:The growth is the
PUSH2→PUSH3widening of theBASISliteral and flag masks.🤖 Generated with Claude Code