fix: switch precedence of cartridgeChains and provided chains

[edisontim]

No description provided.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
controller-example-next	✅ Ready (Inspect)	Visit Preview	Jun 18, 2025 8:46am
keychain	✅ Ready (Inspect)	Visit Preview	Jun 18, 2025 8:46am
keychain-storybook	✅ Ready (Inspect)	Visit Preview	Jun 18, 2025 8:46am
profile	✅ Ready (Inspect)	Visit Preview	Jun 18, 2025 8:46am

[claude]

Claude finished @edisontim's task —— View job

---

PR Review: Switch Precedence of cartridgeChains and Provided Chains

Reviewing the pull request changes for code quality, potential issues, and best practices.

Todo List:

• Read and analyze the modified file: packages/controller/src/controller.ts
• Examine the specific changes made (chain precedence logic)
• Check for potential bugs or issues
• Evaluate code quality and best practices
• Assess performance considerations
• Review security implications
• Consider test coverage needs
• Provide comprehensive review feedback

Changes Summary:

1. Chain precedence fix: Switched array spread order from [...options.chains, ...cartridgeChains] to [...cartridgeChains, ...options.chains] at packages/controller/src/controller.ts:52
2. Local development support: Added localhost exception to RPC validation for mainnet/sepolia at packages/controller/src/controller.ts:441-446

---

✅ Review Assessment: APPROVED

This PR correctly fixes a bug where the code behavior didn't match the documented intent.

Positive Aspects:

1. 🐛 Bug Fix: The change correctly aligns the implementation with the documented behavior. The comment on line 51 clearly states "User chains take precedence if they specify the same network", and this change makes that true.

2. 🏠 Development Support: Adding localhost exception (|| isLocalhost) is practical for local development, allowing developers to use local RPC providers for mainnet/sepolia during testing.

3. 📝 Code Quality: The changes are minimal, focused, and don't introduce complexity. The existing comment accurately describes the intended behavior.

Considerations:

1. ⚠️ Breaking Change: This is potentially a breaking change for users who were relying on the previous behavior where Cartridge chains took precedence. However, since the previous behavior contradicted the documented intent, this is the correct fix.

2. 🧪 Test Coverage Gap: The changes aren't covered by existing tests. The test suite in controllerDefaults.test.ts doesn't verify chain precedence when both cartridge and user chains are provided for the same network.

3. 🔒 Security: The localhost exception is secure as it only applies to development environments and doesn't weaken production security.

Recommendations:

1. Add Test Coverage: Consider adding a test case like:

   test("should prioritize user chains over cartridge chains for same network", () => {
     const userChains = [
       { rpcUrl: "https://api.cartridge.gg/x/my-custom/mainnet" }
     ];
     const controller = new ControllerProvider({
       chains: userChains,
       defaultChainId: constants.StarknetChainId.SN_MAIN,
     });
     expect(controller.rpcUrl()).toBe("https://api.cartridge.gg/x/my-custom/mainnet");
   });

2. Migration Note: If this is a breaking change, consider documenting it in release notes for users who might be affected.

Technical Analysis:

• Performance: ✅ No performance impact
• Security: ✅ Localhost exception is appropriate for development
• Maintainability: ✅ Code is clean and well-documented
• Correctness: ✅ Fixes the documented behavior mismatch

The implementation works correctly because:

1. Arrays are spread in order: [...cartridgeChains, ...options.chains]
2. initializeChains() uses Map.set(chainId, chain) which overwrites duplicates
3. Later chains (user chains) override earlier ones (cartridge chains) for the same chainId

---

[JunichiSugiura]

LGTM !

[claude]

Claude finished @edisontim's task —— View job • claude/pr-1769-20250618_090830 • [Create PR ➔](https://github.com/cartridge-gg/controller/compare/main...claude/pr-1769-20250618_090830?quick_pull=1&title=PR%20%231769%3A%20Changes%20from%20Claude&body=This%20PR%20addresses%20pr%20%231769%0A%0AGenerated%20with%20%5BClaude%20Code%5D(https%3A%2F%2Fclaude.ai%2Fcode)

---

Documentation Analysis for Chain Precedence Fix

Analyzed the controller changes and updated documentation accordingly.

Todo List:

• Read and understand the controller changes in packages/controller/src/controller.ts
• Explore docs-repo directory structure to understand current documentation
• Analyze the chain precedence change to determine if it affects user-facing behavior
• Check if documentation updates are needed for the precedence change
• Update documentation files if necessary
• Commit any documentation changes

Analysis of Controller Changes:

The change in packages/controller/src/controller.ts:52 modifies chain precedence logic:

• Current: const chains = [...cartridgeChains, ...(options.chains || [])];
• Effect: User-provided chains now correctly take precedence over Cartridge default chains when they specify the same network
• Comment confirms intent: "User chains take precedence if they specify the same network" (line 51)

This is a bug fix that aligns implementation with documented behavior. The initializeChains function uses Map.set() which means later chains (user chains) override earlier ones (cartridge chains) for the same chainId.

Documentation Updates Completed:

✅ Updated docs-repo/src/pages/controller/configuration.md:

• Added detailed "Chain Configuration" section explaining default Cartridge RPC endpoints
• Documented chain precedence behavior: user chains override defaults for same network
• Provided example showing how to override default chains and add custom ones
• Updated inline comment for chains option to clarify precedence

✅ Committed changes (SHA: 6b802b4e52ded9f75b9fdd039fe5e6dfc229b14d)

The documentation now clearly explains how the chain configuration works, helping users understand the precedence behavior that was fixed in the original PR.

---