Add integrator fee#72
Conversation
# Conflicts: # package.json # pnpm-lock.yaml
rharutyunyan
left a comment
rharutyunyan
left a comment
There was a problem hiding this comment.
Reviewed and verified locally on the PR head (a617b36): pnpm test (67/67), pnpm lint:ci and tsc --noEmit are all clean.
Also re-derived the load-bearing fixtures independently:
id('my-dapp') → 0xaba10994,id('aba10994') → 0xb5e948e6— the bare-hex APIsourcereally does get keccak-hashed (not used verbatim), exactly as the tests pin and the README warns.extractTrackCodezero-pads to 8 hex chars (0x00000000for empty track).Bps.fromSharePercent(84.45) → 8445,0.125 → 12(truncation), negatives rejected — README claims match the implementation.- Missing
integratorFeeSharePercentwith fee > 0 →IntegratorFee.ZERO, which is compatible with theIntegratorFee/Feesconstructor invariants (explicitshare: 0is still honored as "all to protocol"). - The explicit-salt path checks out:
verifySaltpasses because both orders share the same extension hash, and track bits [224, 256) can never collide with the extension-hash lower 160 bits, sosetSourceafterLimitOrderWithFeeconstruction is safe.
Nothing blocking — a few small nits inline.
| const order = new LimitOrderWithFee(orderInfo, makerTraits, feeExt) | ||
|
|
||
| // Apply API-resolved track code when salt is auto-built | ||
| if (orderInfo.salt === undefined) { |
There was a problem hiding this comment.
Tiny edge-case inconsistency: this check is === undefined, but the LimitOrder constructor treats the salt as falsy (orderInfo.salt || LimitOrder.buildSalt(...)). So for salt: 0n the order silently gets a fresh random salt, yet the API source is not applied (0n !== undefined) — neither the caller's salt nor the track code wins. if (!orderInfo.salt) would match the constructor's semantics. Non-blocking, salt: 0n is a pathological input.
| * E.g. 84.45% → 8445 bps | ||
| */ | ||
| public static fromSharePercent(val: number | string, base = 1n): Bps { | ||
| const normalized = typeof val === 'number' ? val.toString() : val.trim() |
There was a problem hiding this comment.
Since this is now public API accepting strings, note it parses malformed input silently: '1.2.3' → 120 bps, '' → 0 bps. And numbers that stringify to scientific notation (1e-7, 1e21) surface as a raw SyntaxError: Cannot convert 1e-7 to a BigInt instead of the invalid bps assert. A shape check up front would tighten both, e.g. assert(/^\d+(\.\d+)?$/.test(normalized), ...) — it also subsumes the negative check. Non-blocking: the SDK call sites only pass API numbers.
| { | ||
| "name": "@1inch/limit-order-sdk", | ||
| "version": "5.3.1", | ||
| "version": "5.3.2-rc.0", |
There was a problem hiding this comment.
Semver nit: this ships new public API (LimitOrder.getTrackCode, Bps.fromSharePercent, new FeeInfoDTO fields) and changes Sdk.createOrder output for keys with portal fee config — that reads as 5.4.0-rc.0 rather than a patch. A minor bump also makes the behavior change harder to miss for integrators skimming upgrade diffs.
| uses: actions/setup-node@v3 | ||
| with: | ||
| node-version: 20 | ||
| node-version: 22 |
There was a problem hiding this comment.
CI now runs Node 22 only, while engines still advertises >=20.9.0 (and volta pins 20.9.0) — the minimum supported version is no longer exercised. Either keep a 20.x job (matrix) or bump engines. Related: pnpm@11.0.0-rc.1 below is an RC pin for the audit step — worth a TODO to move to stable pnpm 11 once released.
3 participants
Change Summary
What does this PR change?
Add integrator fee
Related Issue/Ticket:
https://1inch.atlassian.net/browse/PT9-759
Testing & Verification
How was this tested?
Risk Assessment
Risk Level: