You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
No specific security vulnerabilities such as SQL injections, XSS, CSRF, etc., are introduced in this PR. However, the lack of validation in the delete_army functions could lead to unauthorized or unintended deletions, which is a security concern in terms of data integrity and application stability.
⚡ Key issues to review
Possible Bug The delete_army method in EternumProvider does not check if the army is a defensive army or if it is alive before deletion. This could lead to unintended behavior where important armies could be deleted or armies could be deleted while still active.
Security Concern The army_delete function in ICombatContract does not perform checks to ensure that the army is not a defensive army and is actually dead before allowing deletion. This could lead to potential misuse where armies could be deleted incorrectly.
Incomplete Implementation The delete_army function in CombatContractImpl does not handle the cleanup of all related entities and states associated with the army, such as removing the army's resources, updating related counters, or cleaning up any pending operations that involve the army.
Why: Adding error handling is crucial for managing exceptions or failed transactions gracefully, which improves the robustness and reliability of the method.
9
Ensure consistent use of the new trait implementation in method calls
Refactor the method calls to use the new trait implementation consistently across all method invocations.
Why: Ensuring consistent use of the new trait implementation is a good practice and can prevent potential confusion or errors in the future.
7
Ensure the new configuration fields are properly validated and do not conflict with existing settings
Review and possibly refactor the addition of the army_free_per_structure and army_extra_per_building fields to ensure they align with the system's overall configuration and do not introduce redundancy or conflicts with existing configurations.
Why: Adding validation for the new fields army_free_per_structure and army_extra_per_building is a good practice to ensure data integrity and avoid conflicts. However, the specific validation logic may need further refinement.
6
Verify and potentially adjust the order of new array elements to maintain logical consistency
Consider maintaining the order of elements in the array to ensure consistent data handling and potentially prevent issues related to data processing order.
-"QuantityTracker",+"QuantityTracker", # Ensure this new element's position is intentional and does not disrupt existing order-dependent logic.
Suggestion importance[1-10]: 6
Why: While maintaining the order of elements is a good practice, the suggestion does not address a critical issue and is more of a precautionary measure.
6
Possible bug
Add validation for non-zero denominators to prevent runtime errors
Ensure that the input parameters are validated for non-zero values before performing calculations to prevent division by zero errors.
+assert(fee_rate_denom != 0, 'fee_rate_denom must be > zero');
let input_amount_after_fee = (input_amount * (fee_rate_denom - fee_rate_num)) / fee_rate_denom;
Suggestion importance[1-10]: 9
Why: Adding validation for non-zero denominators is crucial to prevent runtime errors, which can lead to application crashes or unexpected behavior.
9
Add error handling for data retrieval to prevent runtime failures
Implement error handling for the get! macro to manage cases where the requested data might not be available in the world state, which could lead to runtime errors.
Why: Implementing error handling for the get! macro is crucial for preventing runtime errors when the requested data is unavailable, enhancing the overall stability of the code.
8
Add a safe method to handle potential null or default tick configurations
Replace the direct use of TickImpl::get_default_tick_config with a method that checks for null or default values to prevent runtime errors when the configuration is not set.
Why: This suggestion improves robustness by preventing potential runtime errors due to null or default values, but it requires the implementation of the safe_get_default_tick_config method, which is not trivial.
7
Maintainability
Simplify the array initialization with repeated values
Consider using a loop or a more concise method to initialize the array with repeated values to improve readability and maintainability.
Why: The suggestion improves code readability and maintainability by using a more concise method for array initialization, assuming the syntax is supported by Cairo.
8
Rename variables to maintain consistent camelCase naming conventions
Ensure consistency in naming conventions by renaming army_free_per_structure and army_extra_per_military_building to match the camelCase format used in other parts of the code.
-loop {- if count == production_config.input_count {- break;- }- let production_input: ProductionInput = get!(world, (produced_resource_type, count), ProductionInput);- let (input_resource_type, input_resource_amount) = (- production_input.input_resource_type, production_input.input_resource_amount- );- let mut input_resource: Resource = ResourceCustomImpl::get(- world, (self.outer_entity_id, input_resource_type)- );- let mut input_production: Production = get!(- world, (self.outer_entity_id, input_resource_type), Production- );- input_production.increase_consumption_rate(ref input_resource, @tick, input_resource_amount);- count += 1;- input_resource.save(world);- set!(world, (input_production));-}+handle_resource_consumption(world, production_config, produced_resource_type, self.outer_entity_id, @tick);
Suggestion importance[1-10]: 6
Why: Refactoring the loop into a separate function improves code readability and maintainability, but it does not address any critical issues.
6
Robustness
Add fallback values for new configuration properties to ensure robustness
Consider adding a default value or a fallback mechanism for new configuration properties armyFreePerStructure and armyExtraPerMilitaryBuilding to handle cases where they might not be set.
Why: Adding default values or fallback mechanisms for new configuration properties ensures that the system remains robust even if these properties are not set.
8
Possible issue
Update the class_hash and original_class_hash to unique values to ensure correct deployment tracking
Ensure that the class_hash and original_class_hash values are unique and correctly represent the new deployment to avoid conflicts or incorrect linkage in the contract's deployment history.
Why: Ensuring that class_hash and original_class_hash values are unique is crucial for avoiding conflicts and maintaining accurate deployment records. This suggestion addresses a potential issue in the PR.
8
Enhancement
Implement validation checks for army_id in the army_delete function to ensure robust error handling
Add error handling or validation checks for the new army_delete function to ensure that the input army_id is valid and exists before performing deletion operations.
Why: Adding validation checks for army_id in the army_delete function improves robustness and error handling, although the exact implementation may vary. This suggestion enhances the functionality.
7
Use a constant array for directions to improve readability and maintainability
Use a constant for the direction values in get_bonus_from method to avoid magic numbers and improve code readability.
Why: Using a constant array for directions enhances code readability and maintainability, but the improvement is relatively minor and does not address any functional issues.
5
Documentation
Add documentation for the ArmyDeleteProps interface
Add type documentation for the new ArmyDeleteProps interface to clarify the purpose and usage of its properties.
+/**+ * Properties required to delete an army.+ * @param army_id The identifier of the army to be deleted.+ */
export interface ArmyDeleteProps extends SystemSigner {
army_id: num.BigNumberish;
}
Suggestion importance[1-10]: 6
Why: Adding type documentation helps clarify the purpose and usage of properties, which is beneficial for future developers, though it is not critical.
* Change to V1 + add type aliases for entity ids
* Make elements clickable under right and left navigation modules buttons + fix z-index of quests, guilds and leaderboard
* scarb fmt
* Change bonus_percent to u32 instead of u128
* Address minor PR comments
* prettier
* knip
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Type
Enhancement, Bug fix, Tests
Description
delete_armymethod and related functionality to theEternumProviderand system calls.setCombatConfigandset_troop_configto include new parameters.ArmyQuantityTrackerand updated various combat models and traits to custom versions.army_deletefunction toICombatContractand implemented logic for deleting armies.Changes walkthrough 📝
10 files
index.ts
Add delete army method and update troop configsdk/packages/eternum/src/provider/index.ts
delete_armymethod toEternumProvider.set_troop_configmethod to include new parameters.index.ts
Update setCombatConfig with new parameterssdk/packages/eternum/src/config/index.ts
setCombatConfigto include new parameters.createSystemCalls.ts
Add delete army function to system callsclient/src/dojo/createSystemCalls.ts
delete_armyfunction to system calls.provider.ts
Add ArmyDeleteProps interface and update SetTroopConfigPropssdk/packages/eternum/src/types/provider.ts
ArmyDeletePropsinterface.SetTroopConfigProps.global.ts
Add new global configuration constantssdk/packages/eternum/src/constants/global.ts
combat.cairo
Add ArmyQuantityTracker and update combat modelscontracts/src/models/combat.cairo
ArmyQuantityTrackerimplementation.contracts.cairo
Add army delete function and update combat systemcontracts/src/systems/combat/contracts.cairo
army_deletefunction toICombatContract.army_createandarmy_buy_troopsfunctions.buildings.cairo
Update building models and traitscontracts/src/models/buildings.cairo
contracts.cairo
Update configuration systems to custom implementationscontracts/src/systems/config/contracts.cairo
quantity.cairo
Add QuantityTrackerType and update Quantity implementationcontracts/src/models/quantity.cairo
QuantityTrackerTypemodule.Quantityimplementation to custom version.2 files
deploy.sh
Update deployment script for new versionscripts/deploy.sh
contracts.sh
Update block time in katana commandscripts/contracts.sh
1 files
resource_transfer_system_tests.cairo
Refactor resource transfer system testscontracts/src/systems/resources/tests/resource_transfer_system_tests.cairo
71 files
pnpm-lock.yaml
...pnpm-lock.yaml
...
liquidity.cairo
...contracts/src/systems/bank/tests/liquidity.cairo
...
battle_leave_test.cairo
...contracts/src/systems/combat/tests/battle_leave_test.cairo
...
travel_systems_tests.cairo
...contracts/src/systems/transport/tests/travel_systems_tests.cairo
...
battle_start_test.cairo
...contracts/src/systems/combat/tests/battle_start_test.cairo
...
swap.cairo
...contracts/src/systems/bank/tests/swap.cairo
...
manifest.json
...contracts/manifests/dev/manifest.json
...
tests.cairo
...contracts/src/systems/hyperstructure/tests.cairo
...
resources.cairo
...contracts/src/models/resources.cairo
...
contracts.cairo
...contracts/src/systems/resources/contracts.cairo
...
contracts.cairo
...contracts/src/systems/hyperstructure/contracts.cairo
...
army_create_test.cairo
...contracts/src/systems/combat/tests/army_create_test.cairo
...
tests.cairo
...contracts/src/systems/guild/tests.cairo
...
travel_systems.cairo
...contracts/src/systems/transport/contracts/travel_systems.cairo
...
contracts.cairo
...contracts/src/systems/map/contracts.cairo
...
manifest.toml
...contracts/manifests/dev/manifest.toml
...
trade_systems.cairo
...contracts/src/systems/trade/contracts/trade_systems.cairo
...
army_buy_test.cairo
...contracts/src/systems/combat/tests/army_buy_test.cairo
...
production.cairo
...contracts/src/models/production.cairo
...
contracts.cairo
...contracts/src/systems/guild/contracts.cairo
...
swap.cairo
...contracts/src/systems/bank/contracts/swap.cairo
...
world.cairo
...contracts/src/utils/testing/world.cairo
...
level.cairo
...contracts/src/models/level.cairo
...
config.cairo
...contracts/src/utils/testing/config.cairo
...
manifest.json
...contracts/manifests/prod/manifest.json
...
accept_order.cairo
...contracts/src/systems/trade/tests/trade_systems_tests/accept_order.cairo
...
resource_approval_system_tests.cairo
...contracts/src/systems/resources/tests/resource_approval_system_tests.cairo
...
market.cairo
...contracts/src/models/bank/market.cairo
...
liquidity.cairo
...contracts/src/systems/bank/contracts/liquidity.cairo
...
tests.cairo
...contracts/src/systems/map/tests.cairo
...
create_order.cairo
...contracts/src/systems/trade/tests/trade_systems_tests/create_order.cairo
...
bank.cairo
...contracts/src/systems/bank/contracts/bank.cairo
...
cancel_order.cairo
...contracts/src/systems/trade/tests/trade_systems_tests/cancel_order.cairo
...
tests.cairo
...contracts/src/systems/combat/tests.cairo
...
donkey_systems.cairo
...contracts/src/systems/transport/contracts/donkey_systems.cairo
...
config.cairo
...contracts/src/models/config.cairo
...
position.cairo
...contracts/src/models/position.cairo
...
contracts.cairo
...contracts/src/systems/realm/contracts.cairo
...
road_systems_tests.cairo
...contracts/src/systems/transport/tests/road_systems_tests.cairo
...
realm_leveling_tests.cairo
...contracts/src/systems/leveling/tests/realm_leveling_tests.cairo
...
tests.cairo
...contracts/src/systems/realm/tests.cairo
...
contracts.cairo
...contracts/src/systems/leveling/contracts.cairo
...
systems.cairo
...contracts/src/utils/testing/systems.cairo
...
stamina.cairo
...contracts/src/models/stamina.cairo
...
Scarb.toml
...contracts/Scarb.toml
...
internal_leveling_tests.cairo
...contracts/src/systems/leveling/tests/internal_leveling_tests.cairo
...
road_systems.cairo
...contracts/src/systems/transport/contracts/road_systems.cairo
...
contracts.cairo
...contracts/src/systems/buildings/contracts.cairo
...
road.cairo
...contracts/src/models/road.cairo
...
general.cairo
...contracts/src/utils/testing/general.cairo
...
bank.cairo
...contracts/src/systems/dev/contracts/bank.cairo
...
random.cairo
...contracts/src/utils/random.cairo
...
test-contracts.yml
....github/workflows/test-contracts.yml
...
owner.cairo
...contracts/src/models/owner.cairo
...
hyperstructure_config_tests.cairo
...contracts/src/systems/config/tests/hyperstructure_config_tests.cairo
...
constants.cairo
...contracts/src/constants.cairo
...
resource.cairo
...contracts/src/systems/dev/contracts/resource.cairo
...
vrgda.cairo
...contracts/src/utils/vrgda.cairo
...
math.cairo
...contracts/src/utils/math.cairo
...
structure.cairo
...contracts/src/models/structure.cairo
...
movable.cairo
...contracts/src/models/movable.cairo
...
realm.cairo
...contracts/src/models/realm.cairo
...
guild.cairo
...contracts/src/models/guild.cairo
...
contracts.mdx
...eternum-docs/docs/pages/development/contracts.mdx
...
weight.cairo
...contracts/src/models/weight.cairo
...
capacity.cairo
...contracts/src/models/capacity.cairo
...
contracts.cairo
...contracts/src/systems/name/contracts.cairo
...
population.cairo
...contracts/src/models/population.cairo
...
order.cairo
...contracts/src/models/order.cairo
...
.tool-versions
....tool-versions
...
system_models.json
...contracts/scripts/system_models.json
...