fix(abi): correct encodeMatcherInitPassive wire layout + computeVammQuote skew/rounding - #336
Conversation
…uote skew/rounding
encodeMatcherInitPassive's documented byte layout did not match
InitParams::parse in percolator-match/src/vamm.rs from offset 1 onward.
Concretely: byte[10..14] was written as a literal 100 believing it was
"a default max_inventory_abs slot", but the program reads that range as
max_total_bps — silently capping the LP at a 1% combined spread/fee
ceiling. byte[50..66] was left zero believing it was "reserved", but
the program reads that range as max_inventory_abs, which the matcher
treats 0 as meaning "unlimited" — so every LP set up via this encoder
got unlimited inventory risk with almost no ability to charge spread
to compensate. The only existing test only re-asserted the function's
own (wrong) documented layout, so it never caught the mismatch.
Fixes the field-to-offset mapping and expands MatcherInitPassiveArgs to
actually accept the fields the wire format requires (trading_fee_bps,
base_spread_bps, max_total_bps, max_inventory_abs were previously
unsettable). Rewrites the test to cross-validate every field against
the real offsets, and adds the 78-byte extended-payload path.
Also fixes computeVammQuote, which estimates a vAMM LP's execution
price for client-side LP selection ("Smart Price Router"): it omitted
the on-chain skew-aware inventory widening entirely (VammMatcherParams
had no field for it) and floor-divided the buy-side price instead of
ceiling-dividing like compute_vamm_execution does. Both gaps made every
buy-side quote estimate against a skewed LP systematically lower than
the real on-chain price. Adds inventoryBase/skewSpreadMultBps to
VammMatcherParams, ports compute_skew_extra_bps's exact logic, and
switches the buy-side division to ceiling. No internal consumer
(percolator-keeper, percolator-api) currently calls this function, but
it's publicly exported with a worked example.
Separately: closes the parity-test gap that let the encoder bug ship
undetected. sdk_parity_fixtures.rs already emits the correct
init_field_offsets into specs/matcher-parity.json, but
parity-fixtures.test.ts never read them — it only checked top-level
sizes/magic/self-checks. Adds a test that encodes sentinel values and
verifies each one lands at the exact offset the Rust fixture reports,
so a future layout drift in either direction would be caught
automatically instead of requiring a manual byte-by-byte re-audit.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Unrelated to this fix — avoids pushing a workflow-file change my token lacks the `workflow` OAuth scope for. The matcher.dcccrypto's upstream version of this file will reconcile normally when this branch is rebased/merged there directly rather than via the fork. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe workflow now passes ChangesWorkflow token wiring
Matcher ABI and parity checks
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/parity-gate.yml:
- Line 48: The new actions/checkout usages in the parity-gate workflow are still
pinned to the mutable v4 tag, which violates the action pinning policy. Update
each checkout reference in the workflow to a full commit SHA instead of `@v4`,
making sure all four occurrences are changed consistently so the workflow
remains policy-compliant.
- Around line 47-73: The four actions/checkout steps for percolator-prog,
percolator-stake, percolator-nft, and percolator-match are persisting the token
in git config unnecessarily; update each checkout block to disable credential
persistence. Add the hardening option to every tokenized checkout so the
PERCOLATOR_READ_TOKEN is not retained for later steps, and keep the change
localized to the checkout configuration only.
- Around line 37-44: The Rust checkout steps are unconditionally wiring in the
optional read token, which breaks the public/no-secret path when the secret is
unset. Update the four checkout uses in the parity-gate workflow to only supply
the token when PERCOLATOR_READ_TOKEN exists, and otherwise let actions/checkout
fall back to its default GITHUB_TOKEN behavior. Keep the change scoped to the
checkout steps so the existing public/forked PR flow remains intact.
In `@test/parity-fixtures.test.ts`:
- Around line 272-282: The fixture parity check is missing coverage for the
`impact_k_bps` init field, so an offset regression there could slip through.
Update the parity test in `test/parity-fixtures.test.ts` to also read and assert
`offsets.impact_k_bps` alongside the other init-field offset checks, using the
same `readU32` helper pattern in the existing test block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5bef5717-b847-4907-a675-a30bd122d1f0
⛔ Files ignored due to path filters (3)
dist/abi/instructions.d.tsis excluded by!**/dist/**dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (4)
.github/workflows/parity-gate.ymlsrc/abi/instructions.tstest/abi.test.tstest/parity-fixtures.test.ts
| # If the repo is private, uncomment the token line and add | ||
| # PERCOLATOR_READ_TOKEN as a repository secret on percolator-sdk: | ||
| # Settings → Secrets and variables → Actions → New repository secret | ||
| # Name: PERCOLATOR_READ_TOKEN | ||
| # Value: a GitHub PAT (classic or fine-grained) with Contents: read | ||
| # on percolator-prog, percolator-stake, percolator-nft, percolator-match. | ||
| # If all five repos are public, this token line is not needed. | ||
| # token: ${{ secrets.PERCOLATOR_READ_TOKEN }} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🌐 Web query:
actions/checkout empty token string behavior public repository
💡 Result:
When using the actions/checkout action, explicitly setting the token input to an empty string (token: '') generally causes the checkout process to fail with authentication-related errors [1]. Even for public repositories, the action expects a valid token to perform authenticated Git operations [1]. By default, the action uses the ${{ github.token }} (also known as the GITHUB_TOKEN) provided by the GitHub Actions environment, which is sufficient for checking out public repositories without needing an additional personal access token (PAT) [1][2][3]. If you pass an empty string to the token input, you effectively override this default behavior [2][4]. This often results in Git errors, such as "could not read Username," because the action lacks the necessary credentials to interact with GitHub, and non-authenticated clones may be restricted or subject to stricter rate limiting [1]. In summary: 1. Do not set the token to an empty string if you want the action to succeed using its default behavior [1]. 2. For public repositories, omitting the token input entirely is the correct practice, as it allows the action to fall back to the default ${{ github.token }} [1][2]. 3. The token is also used to configure the local Git environment (persisting credentials), which enables subsequent Git commands in your workflow to run authenticated [2][5][4]. Passing an empty string prevents this configuration from occurring [4].
Citations:
- 1: Do I HAVE TO provide a token for checking out a public repository? actions/checkout#230
- 2: https://github.com/actions/checkout/?tab=readme-ov-file
- 3: https://graphite.com/guides/github-actions-checkout
- 4: https://github.com/actions/checkout/blob/72f2cec99f417b1a1c5e2e88945068983b7965f9/action.yml
- 5: https://github.com/actions/checkout/blob/main/README.md
🌐 Web query:
github actions unset secret evaluates to empty string
💡 Result:
In GitHub Actions, if a secret is unset, not found, or otherwise unavailable to a workflow, the expression referencing that secret evaluates to an empty string ("") [1][2]. This behavior is important for several reasons: 1. Silent Failures: Because an unset secret evaluates to an empty string rather than causing a workflow error, typos in secret names or scoping issues can lead to workflows that run with empty values instead of failing as expected [3]. 2. Conditional Logic: You cannot use secrets directly within if: conditional expressions because they are masked. If you need to check if a secret is set, you should map it to an environment variable first, then check the environment variable in your condition [2][4][5]. Example: env: MY_SECRET_VAL: ${{ secrets.MY_SECRET }} if: ${{ env.MY_SECRET_VAL!= '' }} 3. Fallback Patterns: When using fallback logic (e.g., trying to use one secret or another), you must explicitly check for the empty string. Simple boolean-style fallbacks (like variable || default) may not behave as expected because the unset secret is a string, not a falsy value [6]. Correct fallback example: ${{ secrets.OPTIONAL_TOKEN!= '' && secrets.OPTIONAL_TOKEN || secrets.DEFAULT_TOKEN }} [6] Common reasons a secret may resolve to an empty string unexpectedly include: - Incorrect scope: The secret is defined in an environment that is not explicitly linked to the job [7][5]. - Fork limitations: Workflows triggered by pull requests from forked repositories do not have access to secrets (except for GITHUB_TOKEN) for security reasons [8][9]. - Reusable workflows: When using reusable workflows, secrets must be explicitly passed or inherited using secrets: inherit [8][10].
Citations:
- 1: Document how to use secrets with
if:conditionals in GitHub Actions workflows github/docs#12722 - 2: https://github.com/github/docs/blob/main/content/actions/how-tos/write-workflows/choose-what-workflows-do/use-secrets.md
- 3: https://envmanager.com/blog/github-actions-secrets
- 4: https://stackoverflow.com/questions/72925899/github-actions-detect-if-secret-exists
- 5: https://fixdevs.com/blog/github-actions-secret-not-available/
- 6: Fix token fallback syntax in orchestrator workflow shyamsridhar123/MayorWest#44
- 7: https://stackoverflow.com/questions/67972124/github-return-empty-string-as-secrets-while-running-actions
- 8: https://stackoverflow.com/questions/58737785/github-actions-empty-env-secrets
- 9: Differentiate between input parameter empty or not present actions/runner#924
- 10: [BUG] Environment-scoped secrets unreachable from reusable workflow without secrets: inherit, despite called job declaring environment actions/runner#4453
Preserve the no-secret public checkout path.
The comments state the token is unnecessary for public repositories, but the current implementation unconditionally passes secrets.PERCOLATOR_READ_TOKEN. Unset secrets evaluate to an empty string, and actions/checkout fails if the token input is explicitly provided as an empty string, overriding the default GITHUB_TOKEN behavior required for public/forked PR checkouts.
Apply the suggested fallback to all four Rust checkout steps (lines 52, 59, 66, 73) to ensure GITHUB_TOKEN is used when the optional secret is missing:
Proposed fallback
- token: ${{ secrets.PERCOLATOR_READ_TOKEN }}
+ token: ${{ secrets.PERCOLATOR_READ_TOKEN || github.token }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/parity-gate.yml around lines 37 - 44, The Rust checkout
steps are unconditionally wiring in the optional read token, which breaks the
public/no-secret path when the secret is unset. Update the four checkout uses in
the parity-gate workflow to only supply the token when PERCOLATOR_READ_TOKEN
exists, and otherwise let actions/checkout fall back to its default GITHUB_TOKEN
behavior. Keep the change scoped to the checkout steps so the existing
public/forked PR flow remains intact.
| - name: Checkout percolator-prog | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| repository: ${{ github.repository_owner }}/percolator-prog | ||
| path: percolator-prog | ||
| token: ${{ secrets.PERCOLATOR_READ_TOKEN }} | ||
|
|
||
| - name: Checkout percolator-stake | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| repository: ${{ github.repository_owner }}/percolator-stake | ||
| path: percolator-stake | ||
| token: ${{ secrets.PERCOLATOR_READ_TOKEN }} | ||
|
|
||
| - name: Checkout percolator-nft | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| repository: ${{ github.repository_owner }}/percolator-nft | ||
| path: percolator-nft | ||
| token: ${{ secrets.PERCOLATOR_READ_TOKEN }} | ||
|
|
||
| - name: Checkout percolator-match | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| repository: ${{ github.repository_owner }}/percolator-match | ||
| path: percolator-match | ||
| token: ${{ secrets.PERCOLATOR_READ_TOKEN }} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/parity-gate.yml | sed -n '74,200p'Repository: dcccrypto/percolator-sdk
Length of output: 1840
Disable persisted credentials after tokenized checkouts.
Adding persist-credentials: false to the four actions/checkout steps is a valid security hardening measure. No subsequent steps in this workflow (Install Rust stable, Cache Cargo, Install pnpm, Setup Node 20, Install SDK dependencies, Run parity check) perform git operations that would require authenticated credentials. Disabling persistence minimizes the attack surface by preventing the PERCOLATOR_READ_TOKEN from lingering in the git configuration for unused steps.
Proposed hardening
- token: ${{ secrets.PERCOLATOR_READ_TOKEN }}
+ token: ${{ secrets.PERCOLATOR_READ_TOKEN }}
+ persist-credentials: falseApply this change to the checkout steps for percolator-prog, percolator-stake, percolator-nft, and percolator-match.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 47-52: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 54-59: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 61-66: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 48-48: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 55-55: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 62-62: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 69-69: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/parity-gate.yml around lines 47 - 73, The four
actions/checkout steps for percolator-prog, percolator-stake, percolator-nft,
and percolator-match are persisting the token in git config unnecessarily;
update each checkout block to disable credential persistence. Add the hardening
option to every tokenized checkout so the PERCOLATOR_READ_TOKEN is not retained
for later steps, and keep the change localized to the checkout configuration
only.
Source: Linters/SAST tools
| # repository secrets are not provided to pull_request runs from forks. | ||
| # If they become private, add PERCOLATOR_READ_TOKEN here with Contents: read. | ||
| - name: Checkout percolator-prog | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Pin the new checkout actions to full SHAs.
The added actions/checkout@v4 references violate the configured blanket policy for pinned actions. Pin these four references to full commit SHAs.
#!/bin/bash
# Verify no newly added checkout references remain pinned only to a mutable major tag.
rg -n 'uses:\s*actions/checkout@v[0-9]+\b' .github/workflows/parity-gate.ymlAlso applies to: 55-55, 62-62, 69-69
🧰 Tools
🪛 zizmor (1.26.1)
[error] 48-48: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/parity-gate.yml at line 48, The new actions/checkout
usages in the parity-gate workflow are still pinned to the mutable v4 tag, which
violates the action pinning policy. Update each checkout reference in the
workflow to a full commit SHA instead of `@v4`, making sure all four occurrences
are changed consistently so the workflow remains policy-compliant.
| expect(data[offsets.tag]).toBe(2); | ||
| expect(data[offsets.kind]).toBe(0); | ||
| expect(readU32(offsets.trading_fee_bps)).toBe(11); | ||
| expect(readU32(offsets.base_spread_bps)).toBe(22); | ||
| expect(readU32(offsets.max_total_bps)).toBe(9000); | ||
| expect(readU128(offsets.liquidity_notional_e6)).toBe(0n); | ||
| expect(readU128(offsets.max_fill_abs)).toBe(0x1111_2222_3333_4444n); | ||
| expect(readU128(offsets.max_inventory_abs)).toBe(0x5555_6666_7777_8888n); | ||
| expect(readU16(offsets.fee_to_insurance_bps)).toBe(333); | ||
| expect(readU16(offsets.skew_spread_mult_bps)).toBe(444); | ||
| expect(readU64(offsets.lp_account_id)).toBe(0x9999_AAAA_BBBB_CCCCn); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert impact_k_bps from the fixture too.
This test says it verifies every init field offset, but offsets.impact_k_bps is never read. Since that slot is expected to be zero and sits next to other zero-filled fields, an offset regression there can still pass unnoticed.
Suggested assertion
expect(readU32(offsets.trading_fee_bps)).toBe(11);
expect(readU32(offsets.base_spread_bps)).toBe(22);
expect(readU32(offsets.max_total_bps)).toBe(9000);
+ expect(readU32(offsets.impact_k_bps)).toBe(0);
expect(readU128(offsets.liquidity_notional_e6)).toBe(0n);
expect(readU128(offsets.max_fill_abs)).toBe(0x1111_2222_3333_4444n);
expect(readU128(offsets.max_inventory_abs)).toBe(0x5555_6666_7777_8888n);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(data[offsets.tag]).toBe(2); | |
| expect(data[offsets.kind]).toBe(0); | |
| expect(readU32(offsets.trading_fee_bps)).toBe(11); | |
| expect(readU32(offsets.base_spread_bps)).toBe(22); | |
| expect(readU32(offsets.max_total_bps)).toBe(9000); | |
| expect(readU128(offsets.liquidity_notional_e6)).toBe(0n); | |
| expect(readU128(offsets.max_fill_abs)).toBe(0x1111_2222_3333_4444n); | |
| expect(readU128(offsets.max_inventory_abs)).toBe(0x5555_6666_7777_8888n); | |
| expect(readU16(offsets.fee_to_insurance_bps)).toBe(333); | |
| expect(readU16(offsets.skew_spread_mult_bps)).toBe(444); | |
| expect(readU64(offsets.lp_account_id)).toBe(0x9999_AAAA_BBBB_CCCCn); | |
| expect(data[offsets.tag]).toBe(2); | |
| expect(data[offsets.kind]).toBe(0); | |
| expect(readU32(offsets.trading_fee_bps)).toBe(11); | |
| expect(readU32(offsets.base_spread_bps)).toBe(22); | |
| expect(readU32(offsets.max_total_bps)).toBe(9000); | |
| expect(readU32(offsets.impact_k_bps)).toBe(0); | |
| expect(readU128(offsets.liquidity_notional_e6)).toBe(0n); | |
| expect(readU128(offsets.max_fill_abs)).toBe(0x1111_2222_3333_4444n); | |
| expect(readU128(offsets.max_inventory_abs)).toBe(0x5555_6666_7777_8888n); | |
| expect(readU16(offsets.fee_to_insurance_bps)).toBe(333); | |
| expect(readU16(offsets.skew_spread_mult_bps)).toBe(444); | |
| expect(readU64(offsets.lp_account_id)).toBe(0x9999_AAAA_BBBB_CCCCn); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/parity-fixtures.test.ts` around lines 272 - 282, The fixture parity
check is missing coverage for the `impact_k_bps` init field, so an offset
regression there could slip through. Update the parity test in
`test/parity-fixtures.test.ts` to also read and assert `offsets.impact_k_bps`
alongside the other init-field offset checks, using the same `readU32` helper
pattern in the existing test block.
1 participant
Problem
1.
encodeMatcherInitPassive's documented wire layout didn't match the actual on-chain program. I verifiedInitParams::parseinpercolator-match/src/vamm.rsbyte-for-byte. From offset 1 onward, the function's own layout comment was wrong:[10..14]was written as a literal100, documented as "default max_inventory_abs slot" — the program actually reads this range asmax_total_bps, silently capping the LP at a 1% combined spread/fee ceiling.[50..66]was left zero, documented as "reserved" — the program actually reads this range asmax_inventory_abs, which the matcher treats0as meaning unlimited.Net effect: anyone using this encoder as documented would deploy an LP with unlimited inventory risk and almost no ability to charge spread to compensate — a dangerous silent misconfiguration. The only existing test re-asserted the function's own (wrong) documented layout, so it never caught the mismatch.
2.
computeVammQuote(the "Smart Price Router" quote estimator) systematically underestimates the real on-chain execution price. Two independent gaps, both biased the same direction:VammMatcherParamshad no field for LP inventory/skew at all, so the on-chain skew-aware spread widening (compute_skew_extra_bps) was never modeled; and the buy-side division used floor instead of the on-chain ceiling division ("LP never under-charges").Fix
encodeMatcherInitPassive's field-to-offset mapping and expandedMatcherInitPassiveArgsto actually accept the fields the wire format requires (tradingFeeBps,baseSpreadBps,maxTotalBps,maxInventoryAbswere previously unsettable).inventoryBase/skewSpreadMultBpstoVammMatcherParamsand portedcompute_skew_extra_bps's exact logic intocomputeVammQuote; switched the buy-side division to ceiling.sdk_parity_fixtures.rsalready emits the correctinit_field_offsetsintospecs/matcher-parity.json, butparity-fixtures.test.tsnever read them. Added a test that encodes sentinel values throughencodeMatcherInitPassiveand verifies each lands at the exact offset the Rust fixture reports, so future layout drift in either direction gets caught automatically.dist/(this repo ships pre-built artifacts for git-dependency consumers).No internal consumer (percolator-keeper, percolator-api) currently calls either function, but both are publicly exported with worked examples.
Note on
.github/workflows/parity-gate.ymlThis PR's diff includes an unrelated 15-line change to this file — my branch is based on current upstream
main, but pushing that version to my fork was rejected ("refusing to allow an OAuth App to create or update workflow ... withoutworkflowscope"), so I reverted just that file to my fork's older version to unblock the push. It's unrelated to this fix; feel free to disregard/exclude that hunk when merging.Proof of Fix
encodeMatcherInitPassivefield againstspecs/matcher-parity.json's Rust-sourced offsets (would have caught the original bug).computeVammQuote's skew widening (only applies on the inventory-worsening side, capped at 5000bps) and buy-side ceiling rounding.npm test→ 852 passed, 31 skipped (pre-existing, RPC-dependent).npm run lintclean.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests