fix(core): DojoProvider compatibility with manifests using root level…#516
fix(core): DojoProvider compatibility with manifests using root level…#516MartianGreed merged 1 commit intomainfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds support for root-level ABIs by introducing Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
b1b3d15 to
af1ebc3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/provider/DojoProvider.ts (1)
295-306: MissinggetContractAbi()usage incall()method.The
call()method still directly accessescontractInfos.abi(lines 296 and 301), which will beundefinedfor manifests using root-level ABIs. This is inconsistent with the updates toparseDojoCall()andinitializeActionMethods().🐛 Proposed fix
const contractInfos = getContractByName( this.manifest, nameSpace, call.contractName ); + const abi = getContractAbi(this.manifest, contractInfos); const contract = new Contract({ - abi: contractInfos.abi, + abi, address: contractInfos.address, providerOrAccount: this.provider, }); const compiledCalldata = compileDojoCalldata( - contractInfos.abi, + abi, nameSpace, call.contractName, call.entrypoint, call.calldata );
🧹 Nitpick comments (2)
packages/core/src/provider/DojoProvider.ts (1)
123-127: Consider adding a comment explaining the fallback logic.The fallback chain
manifest.world.abi ?? manifest.abis ?? []passes all manifest ABIs to the world contract whenworld.abiis missing. While this works because the world interface exists withinmanifest.abis, it may include extraneous contract interfaces.A brief comment documenting this intentional behavior for newer manifest formats would improve maintainability.
packages/core/src/_test_/DojoProvider.test.ts (1)
283-323: Consider adding test coverage forcall()method with root-level ABIs.The new test suite validates action method creation and error handling well. However, there's no test verifying that
provider.call()works correctly with root-level ABIs manifests. Given the missinggetContractAbi()integration incall()(flagged separately), adding such a test would help catch this regression.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/support-root-level-abis.mdpackages/core/src/_test_/DojoProvider.test.tspackages/core/src/_test_/manifest_origgun_trail_dev.jsonpackages/core/src/provider/DojoProvider.tspackages/core/src/utils/index.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Apply code formatting with 4 spaces, 80 character line width, double quotes, trailing commas, and semicolons
Files:
packages/core/src/provider/DojoProvider.tspackages/core/src/utils/index.tspackages/core/src/_test_/DojoProvider.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Keep imports grouped: node/external, blank line, workspace, then relative
React hooks/components follow strict dependency arrays and early returns
Prefer guard clauses and rethrow or surface context in error handling; avoid silent catches
Files:
packages/core/src/provider/DojoProvider.tspackages/core/src/utils/index.tspackages/core/src/_test_/DojoProvider.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Maintain explicit return types for exported APIs when inference is unclear
Avoidanytype; lean on generics, discriminated unions, and utility types
Files:
packages/core/src/provider/DojoProvider.tspackages/core/src/utils/index.tspackages/core/src/_test_/DojoProvider.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Tests should remain deterministic; clean up observables/subscriptions between test cases
Files:
packages/core/src/_test_/DojoProvider.test.ts
🧬 Code graph analysis (2)
packages/core/src/provider/DojoProvider.ts (1)
packages/core/src/utils/index.ts (1)
getContractAbi(13-21)
packages/core/src/_test_/DojoProvider.test.ts (1)
packages/core/src/provider/DojoProvider.ts (2)
DojoProvider(507-507)DojoProvider(515-516)
🪛 GitHub Check: build-packages
packages/core/src/_test_/DojoProvider.test.ts
[failure] 5-5: TypeError: vi.importActual is not a function. (In 'vi.importActual("starknet")'
at <anonymous> (/home/runner/work/dojo.js/dojo.js/packages/core/src/_test_/DojoProvider.test.ts:5:29)
at <anonymous> (/home/runner/work/dojo.js/dojo.js/packages/core/src/_test_/DojoProvider.test.ts:4:21)
at /home/runner/work/dojo.js/dojo.js/packages/core/src/_test_/DojoProvider.test.ts:4:4
at loadAndEvaluateModule (2:1)
[failure] 5-5: TypeError: vi.importActual is not a function. (In 'vi.importActual("starknet")'
at <anonymous> (/home/runner/work/dojo.js/dojo.js/packages/core/src/_test_/DojoProvider.test.ts:5:29)
at <anonymous> (/home/runner/work/dojo.js/dojo.js/packages/core/src/_test_/DojoProvider.test.ts:4:21)
at /home/runner/work/dojo.js/dojo.js/packages/core/src/_test_/DojoProvider.test.ts:4:4
at loadAndEvaluateModule (2:1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-examples
- GitHub Check: build
🔇 Additional comments (7)
packages/core/src/utils/index.ts (2)
13-21: Potential logic issue:manifest.abisfallback returns all ABIs, not contract-specific ones.When
contract.abiis missing/empty, the function returnsmanifest.abiswhich contains ABIs for all contracts. This means every contract without inline ABIs will receive the entire ABI collection, which may include unrelated interfaces.Looking at the test manifest (
createRootAbisManifest), all contract systems happen to be defined inmanifest.abis, so it works. However, if contracts have distinct ABIs, this fallback won't properly associate the correct ABI subset with each contract.Is this intentional behavior for the new manifest format (i.e., all contracts share a single ABI array), or should the helper filter ABIs by contract somehow?
67-77: LGTM!The integration of
getContractAbi()inparseDojoCall()is consistent with the new ABI resolution strategy.packages/core/src/provider/DojoProvider.ts (2)
20-20: LGTM!Import addition is correct and follows the grouped import convention.
390-390: LGTM!Correct usage of
getContractAbi()for resolving contract ABIs during action method initialization..changeset/support-root-level-abis.md (1)
1-13: LGTM!The changeset accurately describes the patch and the changes introduced. Well-documented.
packages/core/src/_test_/DojoProvider.test.ts (2)
19-89: LGTM!The
createRootAbisManifest()helper correctly represents the new manifest format with root-level ABIs, providing good test coverage for the fallback logic.
4-15: Current implementation is correct; no changes needed.The code uses
vi.importActual<typeof starknet>("starknet")which is a valid and documented Vitest API for accessing the original module within a mock factory. Bothvi.importActual(path)and theimportOriginal()factory parameter are legitimate approaches per Vitest documentation. The current pattern is correct and follows standard practice.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
af1ebc3 to
2fc13fb
Compare
2fc13fb to
ac5c89c
Compare
… abis
Closes #
Introduced changes
Checklist
Summary by CodeRabbit
New Features
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.