Fix wallet chain id normalization#8784
Conversation
|
@samsamtrum is attempting to deploy a commit to the thirdweb Team on Vercel. A member of the Team first needs to authorize it. |
|
Walkthrough
ChangesChain ID Normalization Validation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
🧹 Nitpick comments (1)
packages/thirdweb/src/wallets/utils/normalizeChainId.test.ts (1)
21-29: ⚡ Quick winGood coverage of invalid inputs.
The test cases effectively verify rejection of malformed strings, NaN, and out-of-range bigints. Consider adding a few more edge cases for completeness:
- Negative number input:
normalizeChainId(-1)- Empty or whitespace-only strings:
normalizeChainId(""),normalizeChainId(" ")- Whitespace handling (positive test):
normalizeChainId(" 1 ")should equal1🧪 Optional additional test cases
it("should reject invalid chain ids", () => { expect(() => normalizeChainId("1abc")).toThrow("Invalid chain ID"); expect(() => normalizeChainId("abc")).toThrow("Invalid chain ID"); expect(() => normalizeChainId("0x")).toThrow("Invalid chain ID"); expect(() => normalizeChainId(Number.NaN)).toThrow("Invalid chain ID"); expect(() => normalizeChainId(BigInt(Number.MAX_SAFE_INTEGER) + 1n), ).toThrow("Invalid chain ID"); + expect(() => normalizeChainId(-1)).toThrow("Invalid chain ID"); + expect(() => normalizeChainId("")).toThrow("Invalid chain ID"); + expect(() => normalizeChainId(" ")).toThrow("Invalid chain ID"); }); + + it("should handle whitespace in string inputs", () => { + expect(normalizeChainId(" 1 ")).toBe(1); + expect(normalizeChainId(" 42 ")).toBe(42); + });🤖 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 `@packages/thirdweb/src/wallets/utils/normalizeChainId.test.ts` around lines 21 - 29, Add tests to cover negative numbers and empty/whitespace-only strings and to assert trimming behavior: in the normalizeChainId test suite add expects that normalizeChainId(-1) throws "Invalid chain ID", that normalizeChainId("") and normalizeChainId(" ") throw "Invalid chain ID", and that normalizeChainId(" 1 ") returns 1 (or does not throw and equals 1). Reference the normalizeChainId test block so these new cases are grouped with the existing "should reject invalid chain ids" tests.
🤖 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.
Nitpick comments:
In `@packages/thirdweb/src/wallets/utils/normalizeChainId.test.ts`:
- Around line 21-29: Add tests to cover negative numbers and
empty/whitespace-only strings and to assert trimming behavior: in the
normalizeChainId test suite add expects that normalizeChainId(-1) throws
"Invalid chain ID", that normalizeChainId("") and normalizeChainId(" ") throw
"Invalid chain ID", and that normalizeChainId(" 1 ") returns 1 (or does not
throw and equals 1). Reference the normalizeChainId test block so these new
cases are grouped with the existing "should reject invalid chain ids" tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dc3524e8-e56f-43b6-80ff-41447afd5e6f
📒 Files selected for processing (2)
packages/thirdweb/src/wallets/utils/normalizeChainId.test.tspackages/thirdweb/src/wallets/utils/normalizeChainId.ts
1 participant
Fixes wallet chain id normalization so malformed decimal strings are rejected instead of being partially parsed. It also rejects NaN, negative, and unsafe numeric values, with regression coverage for invalid chain IDs.
PR-Codex overview
This PR enhances the
normalizeChainIdfunction to handle various invalid input scenarios by throwing errors. It also improves the logic for processing different types ofchainId, ensuring that only valid chain IDs are accepted.Detailed summary
normalizeChainId.test.tsto check for invalidchainIdinputs.normalizeChainIdfunction innormalizeChainId.tsto:normalizedChainIdvariable for cleaner logic.bigintinputs to ensure they are within safe limits.Summary by CodeRabbit
Tests
Bug Fixes