Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughWhitespace-only change in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
|
@CodeRabbit review this plz |
|
@ExistentialEnso, I'll review the changes in this PR right away! ✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/contracts/test/utils/SetupTemplate.t.sol (1)
853-860: Consider a more maintainable approach for identifying Mochi items.The hardcoded index checks create a maintenance burden. Any new Mochi item would require updates in multiple locations (here and in
KamiUseItemSystem._isMochi).Consider alternatives such as:
- Using an item flag (e.g.,
IS_MOCHI) that can be checked viaLibItem- Using a type-based approach if all mochi share a common subtype
- Maintaining a registry or mapping of mochi indices
This aligns with the TODO comment you added in
KamiUseItemSystem.solline 71.Alternative approach using item flags
In
_createFood, instead of checking specific indices:- if ( - index == 11110 || // Gaokerena Mochi - index == 11120 || // Sunset Apple Mochi - index == 11130 || // Kami Mochi - index == 11140 // Mana Mochi - ) { - __ItemRegistrySystem.addAlloBasic(abi.encode(index, "USE", "MOCHI_USED", 0, 1)); - }You could add a flag when creating mochi items and check it instead:
// When creating a mochi item, add a flag __ItemRegistrySystem.addFlag(index, "IS_MOCHI");Then in
KamiUseItemSystem._isMochi:function _isMochi(uint32 itemIndex) internal view returns (bool) { uint256 itemID = LibItem.getByIndex(components, itemIndex); return LibFlag.has(components, itemID, "IS_MOCHI"); }This would eliminate the need for hardcoded indices in both locations.
packages/contracts/src/systems/KamiUseItemSystem.sol (1)
42-45: Extract the magic number to a constant.The mochi limit of
3is hardcoded on line 44. Consider extracting it to a named constant for better maintainability and clarity.♻️ Suggested refactor
uint256 constant ID = uint256(keccak256("system.kami.use.item")); string constant MOCHI_USED_TYPE = "MOCHI_USED"; +uint256 constant MOCHI_LIMIT = 3; // eat one snack contract KamiUseItemSystem is System { ... if (_isMochi(itemIndex)) { uint256 used = LibData.get(components, kamiID, 0, MOCHI_USED_TYPE); - require(used < 3, "mochi limit reached"); + require(used < MOCHI_LIMIT, "mochi limit reached"); }packages/contracts/test/systems/Items/ItemKami.t.sol (1)
83-100: Test correctly verifies mochi limit behavior.The test logic is sound:
- Exercises the happy path (3 successful uses)
- Verifies the limit enforcement (4th use reverts)
- Confirms the counter value (MOCHI_USED equals 3)
Optional improvements for more comprehensive coverage
1. Use the constant instead of string literal (line 98):
+import { MOCHI_USED_TYPE } from "systems/KamiUseItemSystem.sol"; contract ItemKamiTest is ItemTemplate { ... - uint256 used = LibData.get(components, kamiID, 0, "MOCHI_USED"); + uint256 used = LibData.get(components, kamiID, 0, MOCHI_USED_TYPE); assertEq(used, 3); }2. Consider testing that the limit is per-Kami:
function testMochiLimitIsPerKami() public { uint32 mochiA = 11110; _createFood(mochiA, "Gaokerena Mochi", "desc", 10, 0, ""); uint256 kamiID1 = _mintKami(alice); uint256 kamiID2 = _mintKami(alice); _giveItem(alice, mochiA, 6); vm.startPrank(alice.operator); // Use 3 mochi on first kami _KamiUseItemSystem.executeTyped(kamiID1, mochiA); _KamiUseItemSystem.executeTyped(kamiID1, mochiA); _KamiUseItemSystem.executeTyped(kamiID1, mochiA); // Should still be able to use 3 on second kami _KamiUseItemSystem.executeTyped(kamiID2, mochiA); _KamiUseItemSystem.executeTyped(kamiID2, mochiA); _KamiUseItemSystem.executeTyped(kamiID2, mochiA); vm.stopPrank(); assertEq(LibData.get(components, kamiID1, 0, "MOCHI_USED"), 3); assertEq(LibData.get(components, kamiID2, 0, "MOCHI_USED"), 3); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/contracts/deployment/world/data/items/allos.csvis excluded by!**/*.csvpackages/contracts/deployment/world/data/items/items.csvis excluded by!**/*.csv
📒 Files selected for processing (3)
packages/contracts/src/systems/KamiUseItemSystem.solpackages/contracts/test/systems/Items/ItemKami.t.solpackages/contracts/test/utils/SetupTemplate.t.sol
🔇 Additional comments (1)
packages/contracts/src/systems/KamiUseItemSystem.sol (1)
71-78: Hardcoded indices are acceptable for now but need careful synchronization in future refactoring.Your TODO comment appropriately flags a maintenance concern. The indices (11110, 11120, 11130, 11140) are currently synchronized across three locations:
KamiUseItemSystem._isMochi(),SetupTemplate._createFood(), and test files. These must remain in sync when adding new mochi items, so this temporary implementation is acceptable given the explicit TODO.For future refactoring, consider extracting these indices into a central constant or enum that all locations can reference.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/contracts/test/utils/SetupTemplate.t.sol (1)
853-862: LGTM! The mochi limit logic correctly enforces 3 uses per kami.The threshold value of
2with theCURR_MAXcheck correctly allows exactly 3 mochi uses (checks pass when MOCHI_USED is 0, 1, or 2, blocking the 4th use when MOCHI_USED reaches 3).While the hardcoded indices align with your noted implementation approach, consider extracting the mochi indices and limit threshold as constants for easier maintenance:
♻️ Optional refactor for maintainability
// At the top of the contract or in a constants file uint32 constant GAOKERENA_MOCHI_INDEX = 11110; uint32 constant SUNSET_APPLE_MOCHI_INDEX = 11120; uint32 constant KAMI_MOCHI_INDEX = 11130; uint32 constant MANA_MOCHI_INDEX = 11140; uint32 constant MOCHI_LIMIT_THRESHOLD = 2; // Allows 3 uses (0, 1, 2 pass; 3 blocked)Then update the condition:
- if ( - index == 11110 || // Gaokerena Mochi - index == 11120 || // Sunset Apple Mochi - index == 11130 || // Kami Mochi - index == 11140 // Mana Mochi - ) { + if ( + index == GAOKERENA_MOCHI_INDEX || + index == SUNSET_APPLE_MOCHI_INDEX || + index == KAMI_MOCHI_INDEX || + index == MANA_MOCHI_INDEX + ) { __ItemRegistrySystem.addFlag(index, "IS_MOCHI"); __ItemRegistrySystem.addAlloBasic(abi.encode(index, "USE", "MOCHI_USED", 0, 1)); - __ItemRegistrySystem.addRequirement(abi.encode(index, "USE", "MOCHI_USED", "CURR_MAX", 0, 2, "")); + __ItemRegistrySystem.addRequirement(abi.encode(index, "USE", "MOCHI_USED", "CURR_MAX", 0, MOCHI_LIMIT_THRESHOLD, "")); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/contracts/deployment/world/data/items/items.csvis excluded by!**/*.csvpackages/contracts/deployment/world/data/items/requirements.csvis excluded by!**/*.csv
📒 Files selected for processing (3)
packages/contracts/src/systems/KamiUseItemSystem.solpackages/contracts/test/systems/Items/ItemKami.t.solpackages/contracts/test/utils/SetupTemplate.t.sol
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/contracts/src/systems/KamiUseItemSystem.sol
- packages/contracts/test/systems/Items/ItemKami.t.sol
1 participant
Rough implementation of adding a 3 mochi limit to each kami. This is reliant on hardcoded indices for the mochi and could be cleaner. However, it should work as expected. Per @chlwys,
MOCHI_USEDdata entities track the number of mochi used by that kami.Also the diff seems to be acting as if some lines were modified that weren't? Perhaps an encoding issue. Will look at it more deeply soon.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.