Skip to content

fix(merkle): use rails-style cid[N] query, not repeated bare cid#17

Open
niconiconi wants to merge 1 commit into
mainfrom
fix/merkle-query-bracket-format
Open

fix(merkle): use rails-style cid[N] query, not repeated bare cid#17
niconiconi wants to merge 1 commit into
mainfrom
fix/merkle-query-bracket-format

Conversation

@niconiconi

Copy link
Copy Markdown
Contributor

Bug

`freezer.{chain}.2.o.cash/api/v1/merkle?cid=6067&cid=5055&cid=7792` returns:

```
HTTP 400 Multiple values for one key: "cid"
```

The freezer service uses `serde_qs` which by default parses array values as `cid[N]=...`, not as repeated bare keys. The SDK's helper builds the latter, so every multi-cid merkle proof request fails. The frontend's `MerkleManager.ts` uses the correct bracket notation and works; only this SDK helper was the outlier.

Fix

`src/merkle/merkleClient.ts`: rename `withRepeatedQuery` → `withIndexedQuery`, output `cid%5B0%5D=6067&cid%5B1%5D=5055&cid%5B2%5D=7792`.

Tests

  • `tests/merkleClient.test.ts`: regression — asserts URL contains `cid%5B0%5D=...&cid%5B1%5D=...&cid%5B2%5D=...` and does NOT match `[?&]cid=\d+&cid=\d+`.
  • `tests/merkleEngine.test.ts`: existing assertion was locking in the broken format (`?cid=7`). Updated to `?cid%5B0%5D=7` plus renamed test.

124/124 tests pass.

Release

Cut as 0.1.6 once merged. SDK consumers (automation, app via SDK path) will pick up the fix on dep bump.

🤖 Generated with Claude Code

The freezer service uses serde_qs to parse query params, which by
default expects array values as `cid[0]=...&cid[1]=...&cid[2]=...`
(rails-style). Sending repeated bare keys `cid=...&cid=...&cid=...`
(rack-style) returns:

  HTTP 400 Multiple values for one key: "cid"

Every SDK consumer hitting `freezer.{chain}.2.o.cash/api/v1/merkle`
fails on multi-cid requests — automation on Sepolia, the in-app SDK
path, etc. The frontend's MerkleManager already uses bracket notation
and works; only this SDK helper was the outlier.

Rename the helper from `withRepeatedQuery` to `withIndexedQuery` and
build query strings as `cid[N]=value` to match server expectations.

Update merkleEngine.test.ts assertion (was locking in the broken
format) and add a regression test in merkleClient.test.ts that
explicitly checks the URL contains `cid%5B0%5D=...&cid%5B1%5D=...`
and does NOT match `[?&]cid=\d+&cid=\d+`.

124/124 tests pass.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the Merkle client to use rails-style bracket notation for query parameters (e.g., cid[0]=1) instead of repeated keys, ensuring compatibility with the server's serde_qs parser. Corresponding tests have been added and updated to verify this format. Feedback was provided to improve the robustness of the URL construction by handling cases where the base URL might already contain query parameters to avoid malformed URLs.

Comment on lines +18 to 23
const withIndexedQuery = (url: string, key: string, values: Array<string | number>) => {
const search = new URLSearchParams();
for (const v of values) search.append(key, String(v));
values.forEach((v, i) => search.append(`${key}[${i}]`, String(v)));
const qs = search.toString();
return qs ? `${url}?${qs}` : url;
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The withIndexedQuery function unconditionally appends a ? to the URL. If the baseUrl (or the path joined to it) already contains query parameters, this will result in an invalid URL with multiple ? characters (e.g., https://api.com?key=123?cid[0]=...). While the current usage in getProofByCids might be safe with a simple base URL, making this utility more robust prevents bugs when the SDK is configured with a base URL that includes query parameters (e.g., for authentication or proxying).

Suggested change
const withIndexedQuery = (url: string, key: string, values: Array<string | number>) => {
const search = new URLSearchParams();
for (const v of values) search.append(key, String(v));
values.forEach((v, i) => search.append(`${key}[${i}]`, String(v)));
const qs = search.toString();
return qs ? `${url}?${qs}` : url;
};
const withIndexedQuery = (url: string, key: string, values: Array<string | number>) => {
const search = new URLSearchParams();
values.forEach((v, i) => search.append(key + '[' + i + ']', String(v)));
const qs = search.toString();
return qs ? url + (url.includes('?') ? '&' : '?') + qs : url;
};

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant