feat: combat add cases

[edisontim]

User description

• Fixes claims
• Fixes various small bugs

---

PR Type

Enhancement, Bug fix

---

Description

• Enhanced battle action handlers with loading states.
• Refactored BattleView to handle different battle states using new components.
• Added BattleFinisher component for handling battle completion UI.
• Handled cases where one side of the battle is empty in BattleLabel.
• Simplified setBattleView function signature.
• Refactored Structure type for clarity.
• Simplified EnnemyArmies component props.
• Added battle_claim_and_leave method to EternumProvider.
• Renamed and enhanced BattleStarter component.
• Added useArmyByArmyEntityId hook.
• Added new battle-related system calls.
• Updated StructureCard to use BigInt for entity IDs.
• Added BattleClaimAndLeaveProps interface.
• Added non-null assertion for ownArmySelected.

---

Changes walkthrough 📝

	Relevant files
Enhancement	14 files BattleActions.tsx Enhance battle action handlers with loading states              client/src/ui/modules/military/battle-view/BattleActions.tsx Added Loading enum to manage different loading states. Updated battle action handlers to use the new Loading enum. Improved button states and loading indicators. +49/-31  BattleView.tsx Refactor BattleView to handle different battle states        client/src/ui/modules/military/battle-view/BattleView.tsx Replaced NoOngoingBattle with BattleStarter and BattleFinisher. Updated logic to handle different battle states. Refactored to use useArmyByArmyEntityId hook. +67/-26  BattleFinisher.tsx Add BattleFinisher component for battle completion              client/src/ui/modules/military/battle-view/BattleFinisher.tsx Created BattleFinisher component to handle battle completion UI. Integrated BattleActions and BattleProgressBar. +86/-0    BattleLabel.tsx Handle empty battle sides in BattleLabel                                  client/src/ui/components/worldmap/armies/BattleLabel.tsx Added logic to handle cases where one side of the battle is empty. Integrated battle_leave system call. +33/-4    useUIStore.tsx Simplify setBattleView function signature                                client/src/hooks/store/useUIStore.tsx - Simplified `setBattleView` function signature. +2/-15    useStructures.tsx Refactor Structure type for clarity                                            client/src/hooks/helpers/useStructures.tsx - Refactored `Structure` type to improve clarity. +9/-6      Battle.tsx Simplify EnnemyArmies component props                                        client/src/ui/components/military/Battle.tsx - Simplified `EnnemyArmies` component props. +7/-9      index.ts Add battle_claim_and_leave method to EternumProvider          sdk/packages/eternum/src/provider/index.ts - Added `battle_claim_and_leave` method to `EternumProvider`. +17/-1    BattleStarter.tsx Rename and enhance BattleStarter component                              client/src/ui/modules/military/battle-view/BattleStarter.tsx Renamed NoOngoingBattle to BattleStarter. Added health properties for attacker and defender armies. +7/-4      useArmies.tsx Add useArmyByArmyEntityId hook                                                      client/src/hooks/helpers/useArmies.tsx - Added `useArmyByArmyEntityId` hook. +42/-0    createSystemCalls.ts Add new battle-related system calls                                            client/src/dojo/createSystemCalls.ts Added battle_leave_and_claim, battle_claim, and battle_pillage system calls. +16/-0    StructureCard.tsx Update StructureCard to use BigInt for entity IDs                client/src/ui/components/hyperstructures/StructureCard.tsx Updated setBattleView to use BigInt for entity IDs. Added conditional rendering for MergeTroopsPanel. +8/-6      provider.ts Add BattleClaimAndLeaveProps interface                                      sdk/packages/eternum/src/types/provider.ts - Added `BattleClaimAndLeaveProps` interface. +4/-1      HexagonInformationPanel.tsx Add non-null assertion for ownArmySelected                              client/src/ui/components/worldmap/hexagon/HexagonInformationPanel.tsx - Added non-null assertion for `ownArmySelected`. +1/-1
Formatting	1 files WorldHexagon.tsx Adjust import order in WorldHexagon                                            client/src/ui/components/worldmap/hexagon/WorldHexagon.tsx - Adjusted import order for consistency. +2/-4

---

💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

[vercel[bot]]

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

Name	Status	Preview	Comments	Updated (UTC)
eternum	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 18, 2024 9:26am

[github-actions[bot]]

PR Reviewer Guide 🔍

⏱️ Estimated effort to review [1-5]	4
🧪 Relevant tests	No
🔒 Security concerns	No
⚡ Key issues to review	Possible Bug: The BattleActions.tsx file introduces a new enumeration for loading states, which is a good improvement for readability and maintainability. However, there's a potential issue with the condition checks for disabling buttons. The conditions are based on the loading state being Loading.None, which might not be sufficient if other asynchronous operations affect the UI state. Consider adding additional checks or a more comprehensive state management approach to handle multiple loading conditions simultaneously.
	Refactoring Needed: In BattleView.tsx, the logic for rendering different components based on the battle state is complex and could be simplified. The use of multiple nested ternary operators and extensive conditions makes it hard to read and maintain. Consider breaking down the component into smaller sub-components or using a state management library to handle the complex state transitions more cleanly.
	Data Handling: The conversion of army IDs to BigInt directly in the UI components (e.g., in BattleLabel.tsx and StructureCard.tsx) could lead to potential type errors or inconsistencies. It would be safer to handle these conversions in the data-fetching or context layer to ensure that all components receive data in the correct format.

[github-actions[bot]]

PR Code Suggestions ✨

Category	Suggestion	Score
Security	Add validation for owner data before updating to ensure data integrity ___ **Add validation to check if the structure_army_owner and structure_owner are valid before updating their addresses to prevent unauthorized access or errors.** [contracts/src/systems/combat/contracts.cairo [787-801]](https://github.com/BibliothecaDAO/eternum/pull/952/files#diff-4bdcda25a764215afd907449c2bb112a726bf3281fc1a7e6b9f7f0d4351ddb52R787-R801) ```diff let mut structure_army_owner: Owner = get!(world, structure_army_id, Owner); -structure_army_owner.address = starknet::get_caller_address(); -set!(world, (structure_army_owner)); let mut structure_owner: Owner = get!(world, structure_id, Owner); -structure_owner.address = starknet::get_caller_address(); -set!(world, (structure_owner)); +if structure_army_owner.is_valid() && structure_owner.is_valid() { + structure_army_owner.address = starknet::get_caller_address(); + structure_owner.address = starknet::get_caller_address(); + set!(world, (structure_army_owner)); + set!(world, (structure_owner)); +} else { + throw!("Invalid owner data"); +} ``` Suggestion importance[1-10]: 10 Why: Adding validation checks before updating owner data is essential for preventing unauthorized access and ensuring data integrity, which is a critical security measure.	10
Possible issue	Add error handling to the asynchronous operation in the battle_claim function ___ **Consider adding error handling for the asynchronous operations within the battle_claim function to ensure that any exceptions or rejections are properly managed. This will improve the robustness of the function and provide better feedback or recovery options in case of failures.** [client/src/dojo/createSystemCalls.ts [185-187]](https://github.com/BibliothecaDAO/eternum/pull/952/files#diff-322af6c204272be73084828a2b1bab251422314ba594744850c8659eed13a8faR185-R187) ```diff -console.log(props); -await provider.battle_claim(props); +try { + console.log(props); + await provider.battle_claim(props); +} catch (error) { + console.error('Failed to claim battle:', error); +} ``` Suggestion importance[1-10]: 9 Why: Adding error handling to the `battle_claim` function is crucial for robustness and provides better feedback in case of failures. This suggestion correctly addresses a potential issue by wrapping the asynchronous call in a try-catch block.	9
	Add error handling for transaction execution in the battle_claim_and_leave method ___ **Consider handling exceptions for the executeAndCheckTransaction method within battle_claim_and_leave to ensure that any errors during the transaction execution are caught and handled appropriately.** [sdk/packages/eternum/src/provider/index.ts [508-522]](https://github.com/BibliothecaDAO/eternum/pull/952/files#diff-467989f145beaf5796d645b8891d6c2468a39d539d62b48a1459185c6fe1de55R508-R522) ```diff -return await this.executeAndCheckTransaction(signer, [ - { - contractAddress: getContractByName(this.manifest, "combat_systems"), - entrypoint: "battle_leave", - calldata: [battle_id, army_id], - }, - { - contractAddress: getContractByName(this.manifest, "combat_systems"), - entrypoint: "battle_claim", - calldata: [army_id, structure_id], - }, -]); +try { + return await this.executeAndCheckTransaction(signer, [ + { + contractAddress: getContractByName(this.manifest, "combat_systems"), + entrypoint: "battle_leave", + calldata: [battle_id, army_id], + }, + { + contractAddress: getContractByName(this.manifest, "combat_systems"), + entrypoint: "battle_claim", + calldata: [army_id, structure_id], + }, + ]); +} catch (error) { + console.error('Failed to execute transaction:', error); + throw error; +} ``` Suggestion importance[1-10]: 9 Why: Adding error handling is crucial for ensuring that any issues during transaction execution are caught and managed appropriately, improving the robustness of the method.	9
	Verify compatibility of lockfile version upgrade to prevent potential issues ___ **Ensure that the version updates are necessary and beneficial. The update from lockfileVersion: '6.0' to lockfileVersion: '9.0' might introduce compatibility issues with older versions of the tooling or with other team members' environments. Verify compatibility before upgrading.** [pnpm-lock.yaml [1]](https://github.com/BibliothecaDAO/eternum/pull/952/files#diff-32824c984905bb02bc7ffcef96a77addd1f1602cff71a11fbbfdd7f53ee026bbR1-R1) ```diff -lockfileVersion: '9.0' +lockfileVersion: '6.0' ``` Suggestion importance[1-10]: 7 Why: Verifying compatibility before upgrading the lockfile version is important to prevent potential issues, but the suggestion to revert might not always be necessary if compatibility is confirmed.	7
Possible bug	Add a check for undefined battleId to prevent runtime errors ___ **Consider adding a check for battleId being undefined before using it in the handleBattleClaim function. This will prevent potential runtime errors when battleId is not provided.** [client/src/ui/modules/military/battle-view/BattleActions.tsx [87-93]](https://github.com/BibliothecaDAO/eternum/pull/952/files#diff-a2b38e5f9c040c3591adb945a143c3fe234beb433a77a677a8d069ea47a6a462R87-R93) ```diff -if (battleId! !== 0n && battleId === BigInt(attacker.battle_id)) { +if (battleId && battleId !== 0n && battleId === BigInt(attacker.battle_id)) { await battle_leave_and_claim({ signer: account, army_id: ownArmy!.entity_id, - battle_id: battleId!, + battle_id: battleId, structure_id: structure!.entity_id, }); } ``` Suggestion importance[1-10]: 9 Why: Adding a check for `battleId` being undefined is crucial for preventing potential runtime errors, thus enhancing the robustness of the code.	9
	Ensure atomic updates for structure_army_owner and structure_owner to prevent partial state changes ___ **Ensure that the structure_army_owner and structure_owner updates are atomic by using a transaction mechanism or ensuring that the state changes are committed only after all updates are successful.** [contracts/src/systems/combat/contracts.cairo [787-801]](https://github.com/BibliothecaDAO/eternum/pull/952/files#diff-4bdcda25a764215afd907449c2bb112a726bf3281fc1a7e6b9f7f0d4351ddb52R787-R801) ```diff let mut structure_army_owner: Owner = get!(world, structure_army_id, Owner); +let mut structure_owner: Owner = get!(world, structure_id, Owner); structure_army_owner.address = starknet::get_caller_address(); -set!(world, (structure_army_owner)); -let mut structure_owner: Owner = get!(world, structure_id, Owner); structure_owner.address = starknet::get_caller_address(); -set!(world, (structure_owner)); +// Ensure atomic update +update!(world, [(structure_army_id, structure_army_owner), (structure_id, structure_owner)]); ``` Suggestion importance[1-10]: 8 Why: Ensuring atomic updates is important to maintain data consistency and prevent partial state changes, which could lead to inconsistencies in the system.	8
Enhancement	Ensure type consistency in the BattleViewInfo type definition ___ **Update the BattleViewInfo type definition to ensure consistency in the type of attackers and defenders.entities fields. Both should be arrays of bigint for uniformity and to avoid potential type mismatches.** [client/src/hooks/store/types.ts [4-7]](https://github.com/BibliothecaDAO/eternum/pull/952/files#diff-18fea19cd82ce984dbf868aa7872114b46fa3ab4808a3c8569e93423c050cd41R4-R7) ```diff export type BattleViewInfo = { attackers: bigint[]; - defenders: { type: CombatTarget; entities: bigint[] | Realm | Structure }; + defenders: { type: CombatTarget; entities: bigint[] }; }; ``` Suggestion importance[1-10]: 8 Why: Ensuring type consistency in the `BattleViewInfo` type definition is important for avoiding potential type mismatches. This suggestion improves the type definition for better uniformity and reliability.	8
	Use more descriptive enum names to clarify their purpose ___ **Use a more descriptive variable name than Loading for the enum to clarify its purpose related to UI state management.** [client/src/ui/modules/military/battle-view/BattleActions.tsx [14-20]](https://github.com/BibliothecaDAO/eternum/pull/952/files#diff-a2b38e5f9c040c3591adb945a143c3fe234beb433a77a677a8d069ea47a6a462R14-R20) ```diff -enum Loading { +enum BattleLoadingState { None, - Claim, - Raid, - Start, - Leave, + Claiming, + Raiding, + Starting, + Leaving, } ``` Suggestion importance[1-10]: 6 Why: Using more descriptive enum names improves code readability and clarity, but it is a minor enhancement.	6
Maintainability	Simplify the version specification to avoid potential conflicts and duplication ___ **Consider using a consistent versioning approach for dependencies. The version 8.1.0(@swc/core@1.5.28)(postcss@8.4.38)(typescript@5.4.4) for tsup includes specific versions of dependencies which might not be necessary if they are already managed elsewhere in the project. This can lead to version conflicts or duplication in dependency management.** [pnpm-lock.yaml [13]](https://github.com/BibliothecaDAO/eternum/pull/952/files#diff-32824c984905bb02bc7ffcef96a77addd1f1602cff71a11fbbfdd7f53ee026bbR13-R13) ```diff -version: 8.1.0(@swc/core@1.5.28)(postcss@8.4.38)(typescript@5.4.4) +version: 8.1.0 ``` Suggestion importance[1-10]: 8 Why: Simplifying the version specification can prevent potential conflicts and duplication, improving maintainability.	8
	Simplify nested dependency declarations in version strings ___ **Consider the impact of deeply nested dependencies in version strings, such as version: 1.4.1(rollup@4.18.0)(vite@5.2.13(@types/node@20.14.2)). This might complicate updates and obscure the source of issues. Simplifying these declarations can aid in maintainability and clarity.** [pnpm-lock.yaml [163]](https://github.com/BibliothecaDAO/eternum/pull/952/files#diff-32824c984905bb02bc7ffcef96a77addd1f1602cff71a11fbbfdd7f53ee026bbR163-R163) ```diff -version: 1.4.1(rollup@4.18.0)(vite@5.2.13(@types/node@20.14.2)) +version: 1.4.1 ``` Suggestion importance[1-10]: 8 Why: Simplifying nested dependency declarations can improve maintainability and clarity, making it easier to manage and debug issues.	8
	Refactor repeated code into a function to reduce duplication and improve maintainability ___ **Refactor the repeated setLoading and setBattleView calls in each handler function to a separate function to reduce code duplication and improve maintainability.** [client/src/ui/modules/military/battle-view/BattleActions.tsx [56-64]](https://github.com/BibliothecaDAO/eternum/pull/952/files#diff-a2b38e5f9c040c3591adb945a143c3fe234beb433a77a677a8d069ea47a6a462R56-R64) ```diff -setLoading(Loading.Raid); -await battle_pillage({ +async function handleAction(action, loadingState) { + setLoading(loadingState); + await action(); + setLoading(Loading.None); + setBattleView(null); +} +handleAction(() => battle_pillage({ signer: account, army_id: attacker.entity_id, structure_id: structure!.entity_id, -}); -setLoading(Loading.None); -setBattleView(null); +}), Loading.Raid); ``` Suggestion importance[1-10]: 7 Why: Refactoring repeated code into a separate function reduces duplication and enhances maintainability, although it is not critical, it improves code quality.	7
	Split battle_claim_and_leave into two separate methods for better separation of concerns ___ **Refactor the battle_claim_and_leave method to separate the concerns of claiming and leaving a battle into distinct methods to enhance code readability and maintainability.** [sdk/packages/eternum/src/provider/index.ts [508-523]](https://github.com/BibliothecaDAO/eternum/pull/952/files#diff-467989f145beaf5796d645b8891d6c2468a39d539d62b48a1459185c6fe1de55R508-R523) ```diff -public async battle_claim_and_leave(props: SystemProps.BattleClaimAndLeaveProps) { - const { army_id, structure_id, battle_id, signer } = props; +public async battle_claim(props: SystemProps.BattleClaimProps) { + const { army_id, structure_id, signer } = props; return await this.executeAndCheckTransaction(signer, [ - { - contractAddress: getContractByName(this.manifest, "combat_systems"), - entrypoint: "battle_leave", - calldata: [battle_id, army_id], - }, { contractAddress: getContractByName(this.manifest, "combat_systems"), entrypoint: "battle_claim", calldata: [army_id, structure_id], }, ]); } +public async battle_leave(props: SystemProps.BattleLeaveProps) { + const { battle_id, army_id, signer } = props; + return await this.executeAndCheckTransaction(signer, [ + { + contractAddress: getContractByName(this.manifest, "combat_systems"), + entrypoint: "battle_leave", + calldata: [battle_id, army_id], + }, + ]); +} ``` Suggestion importance[1-10]: 7 Why: While separating the concerns into distinct methods can enhance readability and maintainability, it is not critical and may introduce additional complexity if both actions are often performed together.	7
	Improve readability and maintainability of the handleOneEmptySide function ___ **Refactor the handleOneEmptySide function to improve readability and maintainability by using a more descriptive variable name for the function and its parameters, and by adding type annotations.** [client/src/ui/components/worldmap/armies/BattleLabel.tsx [67-83]](https://github.com/BibliothecaDAO/eternum/pull/952/files#diff-e670763d3ccc2538073b9d1e05dd4a532075551f8f29c5633b3efb6b57e2de95R67-R83) ```diff -const handleOneEmptySide = ( +const handleEmptyBattleSide = ( attackers: ArmyInfo[], defenders: ArmyInfo[], - battle_leave: any, - account: any, + leaveBattle: (params: { signer: any; battle_id: bigint; army_id: bigint }) => void, + userAccount: any, battleId: bigint, ) => { - let ownArmy: ArmyInfo | undefined; - if (attackers.length === 0) { - ownArmy = defenders.find((army) => army.isMine); + let ownArmy = attackers.length === 0 ? defenders.find((army) => army.isMine) : attackers.find((army) => army.isMine); + if (ownArmy) { + leaveBattle({ signer: userAccount, battle_id: battleId, army_id: BigInt(ownArmy.entity_id) }); } - if (defenders.length === 0) { - ownArmy = attackers.find((army) => army.isMine); - } - if (!ownArmy) return; - battle_leave({ signer: account, battle_id: battleId, army_id: ownArmy.entity_id }); }; ``` Suggestion importance[1-10]: 6 Why: The refactoring suggestion improves readability and maintainability by using more descriptive variable names and adding type annotations. However, the functional change is minimal, so the impact is moderate.	6
Best practice	Replace non-null assertions with conditional checks to enhance code safety ___ **Replace the non-null assertion operator (!) with conditional checks to ensure that ownArmy and defender are not undefined before accessing their properties. This will enhance code safety and prevent runtime errors.** [client/src/ui/modules/military/battle-view/BattleActions.tsx [77-81]](https://github.com/BibliothecaDAO/eternum/pull/952/files#diff-a2b38e5f9c040c3591adb945a143c3fe234beb433a77a677a8d069ea47a6a462R77-R81) ```diff -await battle_start({ - signer: account, - attacking_army_id: ownArmy!.entity_id, - defending_army_id: defender!.entity_id, -}); +if (ownArmy && defender) { + await battle_start({ + signer: account, + attacking_army_id: ownArmy.entity_id, + defending_army_id: defender.entity_id, + }); +} ``` Suggestion importance[1-10]: 8 Why: Replacing non-null assertions with conditional checks improves code safety and prevents potential runtime errors, which is a best practice.	8
	Reduce specificity in version declarations to improve manageability ___ **Review the necessity of adding specific sub-dependency versions directly in the version field. For example, version: 0.7.0-alpha.2(starknet@6.7.0(encoding@0.1.13))(typescript@5.4.4) might be overly specific and could be managed better through a centralized dependency management strategy.** [pnpm-lock.yaml [22]](https://github.com/BibliothecaDAO/eternum/pull/952/files#diff-32824c984905bb02bc7ffcef96a77addd1f1602cff71a11fbbfdd7f53ee026bbR22-R22) ```diff -version: 0.7.0-alpha.2(starknet@6.7.0(encoding@0.1.13))(typescript@5.4.4) +version: 0.7.0-alpha.2 ``` Suggestion importance[1-10]: 8 Why: Reducing specificity in version declarations can improve manageability and prevent potential conflicts, making it a good practice.	8
Performance	Refactor the destructuring in useArmyByArmyEntityId to improve performance ___ **Refactor the useArmyByArmyEntityId hook to avoid the potential performance issue caused by the extensive destructuring inside the hook. This can be achieved by directly accessing the required properties instead of destructuring all properties at once.** [client/src/hooks/helpers/useArmies.tsx [252-271]](https://github.com/BibliothecaDAO/eternum/pull/952/files#diff-6d68c14fbe3262def9a16f121115d2006c7c34bf942b3e5d8ed7c076d2b89027R252-R271) ```diff -const { - setup: { - components: { - Position, - EntityOwner, - Owner, - Health, - Quantity, - Movable, - Capacity, - ArrivalTime, - Realm, - Army, - Protectee, - EntityName, - Stamina, - }, - }, - account: { account }, -} = useDojo(); +const dojo = useDojo(); +const { Army, Protectee, EntityName, Health, Quantity, Movable, Capacity, ArrivalTime, Position, EntityOwner, Owner, Realm, Stamina } = dojo.setup.components; +const { account } = dojo.account; ``` Suggestion importance[1-10]: 7 Why: The suggestion improves performance by avoiding extensive destructuring inside the hook. However, the performance gain might be minor in this context, so the impact is not very high.	7