Enh/guilds pop

[cwastche]

User description

Add max capacity to guilds

---

PR Type

Enhancement, Tests

---

Description

• Added creation_ts and join_ts fields to Guild and GuildMember schemas.
• Integrated Population schema with Guild and updated related components and hooks.
• Added methods and interfaces for changing guild access and setting guild population configuration.
• Updated configuration scripts to include guild population settings.
• Added tests for new guild access and population management functionalities.

---

Changes walkthrough 📝

	Relevant files
Enhancement	18 files contractComponents.ts Add timestamps to Guild and GuildMember schemas                    client/src/dojo/contractComponents.ts Added creation_ts and join_ts fields to Guild and GuildMember schemas. Updated types arrays to reflect new fields. +4/-3      useGuilds.tsx Integrate Population schema with Guild data                            client/src/hooks/helpers/useGuilds.tsx Integrated Population schema with Guild. Updated type definitions and queries to use Population. Modified functions to handle population data. +19/-15  GuildInvites.tsx Update GuildInvites to use new GuildWhitelist type              client/src/ui/components/worldmap/guilds/GuildInvites.tsx Updated type imports and usage to reflect new GuildWhitelist type. +6/-6      GuildMembers.tsx Update GuildMembers to use new GuildMember type                    client/src/ui/components/worldmap/guilds/GuildMembers.tsx - Updated type imports and usage to reflect new `GuildMember` type. +6/-6      Guilds.tsx Update Guilds component to show population and capacity    client/src/ui/components/worldmap/guilds/Guilds.tsx Updated type imports and usage to reflect new Guild type. Modified display to show population and capacity. +9/-9      MyGuild.tsx Update MyGuild to use population data                                        client/src/ui/components/worldmap/guilds/MyGuild.tsx - Updated to use `population` instead of `memberCount`. +2/-2      Whitelist.tsx Update Whitelist to use new GuildWhitelist type                    client/src/ui/components/worldmap/guilds/Whitelist.tsx Updated type imports and usage to reflect new GuildWhitelist type. +6/-6      index.ts Add guild population configuration setup                                  config/index.ts - Added `setGuildPopulationConfig` to configuration setup. +2/-0      index.ts Add function to set guild population configuration              sdk/packages/eternum/src/config/index.ts - Added `setGuildPopulationConfig` function. +10/-0    global.ts Add base guild population capacity constant                            sdk/packages/eternum/src/constants/global.ts - Added `BASE_GUILD_POPULATION_CAPACITY` constant. +3/-0      index.ts Add methods for guild access and population configuration sdk/packages/eternum/src/provider/index.ts Added change_guild_access and set_guild_population_config methods. +19/-0    provider.ts Add interfaces for guild access and population configuration sdk/packages/eternum/src/types/provider.ts Added interfaces for ChangeGuildAccess and SetGuildPopulationConfigProps. +9/-0      system_models.json Include Population in GUILD_SYSTEMS                                            contracts/scripts/system_models.json - Added `Population` to `GUILD_SYSTEMS`. +25/-5    constants.cairo Add guild population config ID constant                                    contracts/src/constants.cairo - Added `GUILD_POPULATION_CONFIG_ID` constant. +1/-0      config.cairo Add GuildPopulationConfig model                                                    contracts/src/models/config.cairo - Added `GuildPopulationConfig` model. +8/-0      guild.cairo Add timestamps to Guild and GuildMember models                      contracts/src/models/guild.cairo - Added `creation_ts` to `Guild` and `join_ts` to `GuildMember`. +3/-2      contracts.cairo Add method to set guild population config in config systems contracts/src/systems/config/contracts.cairo - Added `set_guild_population_config` method to `config_systems`. +18/-2    contracts.cairo Add guild access change and population management                contracts/src/systems/guild/contracts.cairo Added change_guild_access method. Integrated population management in guild creation and membership changes. +69/-16
Tests	1 files tests.cairo Add tests for guild access and population management          contracts/src/systems/guild/tests.cairo - Added tests for guild access change and population management. +55/-5

---

💡 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	❌ Failed (Inspect)			Jul 2, 2024 2:29pm

[github-actions[bot]]

PR Reviewer Guide 🔍

⏱️ Estimated effort to review [1-5]	4
🧪 Relevant tests	Yes
🔒 Security concerns	No
⚡ Key issues to review	Possible Bug: The change_guild_access function in contracts/src/systems/guild/contracts.cairo does not check if the is_public parameter is actually different from the current state before updating. This could lead to unnecessary state changes and gas usage.
	Data Integrity: In contracts/src/systems/guild/contracts.cairo, the leave_guild function sets guild_member.guild_entity_id to 0 and guild_member.join_ts to 0 after a member leaves. However, it does not clear other potentially sensitive data related to the member. Consider reviewing what data should be cleared to prevent stale or sensitive data from persisting.
	Error Handling: The join_guild function in contracts/src/systems/guild/contracts.cairo increases the population count without prior checks for maximum capacity until after the increment. This could lead to situations where the population temporarily exceeds the maximum capacity, which might not be intended.

[github-actions[bot]]

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	Add capacity check before allowing a new member to join a guild ___ **Add validation in the join_guild function to check if the guild is already at maximum capacity before allowing a new member to join. This prevents exceeding the guild's population capacity.** [contracts/src/systems/guild/contracts.cairo [101-113]](https://github.com/BibliothecaDAO/eternum/pull/1040/files#diff-df441b7afbca46c8a1c411bcd2c2c28ded66187d59f0a43d8ece0958a5e8645cR101-R113) ```diff fn join_guild(ref world: IWorldDispatcher, guild_entity_id: u128) { let caller_address = starknet::get_caller_address(); let guild = get!(world, guild_entity_id, Guild); if (!guild.is_public) { get!(world, (caller_address, guild_entity_id), GuildWhitelist).assert_is_whitelisted(); } let mut population = get!(world, guild_entity_id, Population); + if population.population >= population.capacity { + throw!("Guild is at maximum capacity"); + } population.increase_population(1, guild_population_config.base_population); population.assert_within_capacity(0); set!(world, (GuildMember { address: caller_address, guild_entity_id: guild_entity_id, join_ts: current_ts }, population)); } ``` Suggestion importance[1-10]: 10 Why: This validation is crucial to prevent exceeding the guild's population capacity, addressing a potential bug.	10
	Ensure guild_member.join_ts is reset to zero when leaving a guild for data integrity ___ **In the leave_guild function, ensure that the guild_member struct is updated correctly by setting join_ts to zero when a member leaves the guild, to maintain data integrity and avoid potential future errors.** [contracts/src/systems/guild/contracts.cairo [143-157]](https://github.com/BibliothecaDAO/eternum/pull/1040/files#diff-df441b7afbca46c8a1c411bcd2c2c28ded66187d59f0a43d8ece0958a5e8645cR143-R157) ```diff fn leave_guild(ref world: IWorldDispatcher) { let caller_address = starknet::get_caller_address(); let mut guild_member = get!(world, caller_address, GuildMember); guild_member.assert_has_guild(); guild_member.guild_entity_id = 0; + guild_member.join_ts = 0; // Reset join timestamp when leaving the guild. set!(world, (guild_member)); } ``` Suggestion importance[1-10]: 9 Why: Ensuring data integrity by resetting `join_ts` is important to avoid potential future errors, making this a significant improvement.	9
	Ensure proper initialization of the creation_ts field to avoid runtime errors ___ **Ensure that the creation_ts field is initialized properly when defining new contract components. This is crucial for maintaining data consistency and avoiding potential runtime errors due to uninitialized values.** [client/src/dojo/contractComponents.ts [991]](https://github.com/BibliothecaDAO/eternum/pull/1040/files#diff-5bab736e704fb6f184e27c05d5a774e9e90c45f019bf5733c9e6ea9efb249101R991-R991) ```diff { entity_id: RecsType.BigInt, is_public: RecsType.Boolean, - creation_ts: RecsType.BigInt, + creation_ts: RecsType.BigInt = getCurrentTimestamp(), // Assuming `getCurrentTimestamp` is a function that fetches the current timestamp } ``` Suggestion importance[1-10]: 8 Why: Proper initialization of the `creation_ts` field is crucial to avoid potential runtime errors. This suggestion addresses a possible bug and enhances data consistency.	8
	Add a type check for population to prevent runtime errors ___ **Add a check to ensure population is defined before comparing its value, to prevent potential runtime errors.** [client/src/ui/components/worldmap/guilds/MyGuild.tsx [177]](https://github.com/BibliothecaDAO/eternum/pull/1040/files#diff-187d62a536d58004f66e023774aeb6b9d3cdda70528f8236c8bc7ee63db1aa23R177-R177) ```diff -{population && population > 1 ? ( +{typeof population === 'number' && population > 1 ? ( ``` Suggestion importance[1-10]: 8 Why: Adding a type check for `population` ensures that the comparison is valid, preventing potential runtime errors and improving code robustness.	8
Possible issue	Handle potential undefined values in population and capacity to ensure data accuracy ___ **Ensure that the population and capacity fields are properly handled to avoid displaying undefined or incorrect data.** [client/src/ui/components/worldmap/guilds/Guilds.tsx [122]](https://github.com/BibliothecaDAO/eternum/pull/1040/files#diff-35e1ce0de5d066ca31a31dee90665f2b4ff706e2473f4e0d65c4472394042cd5R122-R122) ```diff - {guild.population} / {guild.capacity} + {guild.population ?? 0} / {guild.capacity ?? 'N/A'} ``` Suggestion importance[1-10]: 9 Why: Ensuring that `population` and `capacity` fields are properly handled prevents displaying incorrect or undefined data, which is crucial for user interface accuracy.	9
Performance	Optimize the change_guild_access function to avoid unnecessary state changes ___ **Consider checking if the is_public status of the guild actually changes before setting it in the change_guild_access function to avoid unnecessary state changes and potential reverts or gas costs.** [contracts/src/systems/guild/contracts.cairo [238-244]](https://github.com/BibliothecaDAO/eternum/pull/1040/files#diff-df441b7afbca46c8a1c411bcd2c2c28ded66187d59f0a43d8ece0958a5e8645cR238-R244) ```diff fn change_guild_access(ref world: IWorldDispatcher, guild_entity_id: u128, is_public: bool) { get!(world, guild_entity_id, Owner).assert_caller_owner(); let mut guild = get!(world, guild_entity_id, Guild); - guild.is_public = is_public; - set!(world, (guild)); + if guild.is_public != is_public { + guild.is_public = is_public; + set!(world, (guild)); + } } ``` Suggestion importance[1-10]: 8 Why: This optimization can save gas costs and prevent unnecessary state changes, which is beneficial for performance.	8
Maintainability	Use a constant for population checks to enhance code maintainability ___ **Replace the hardcoded population check with a constant or configurable value to enhance flexibility and maintainability of the code.** [client/src/hooks/helpers/useGuilds.tsx [45]](https://github.com/BibliothecaDAO/eternum/pull/1040/files#diff-4a6ae30a3969851b248ce9c56e472b99b3f3ae1a4598fd3610e91aadc0899fa0R45-R45) ```diff -const guilds = useEntityQuery([Has(Guild), NotValue(Population, { population: 0 })]); +const MIN_POPULATION = 1; // Define this constant at a higher scope or configuration file +const guilds = useEntityQuery([Has(Guild), NotValue(Population, { population: MIN_POPULATION })]); ``` Suggestion importance[1-10]: 7 Why: Replacing hardcoded values with constants improves code maintainability and flexibility. This suggestion is beneficial for future modifications and readability.	7
Enhancement	Add a docstring to the GuildPopulationConfig struct to enhance code documentation ___ **It's recommended to add a docstring to the new struct GuildPopulationConfig to explain its purpose and usage within the system. This will help maintain code readability and ease future maintenance.** [contracts/src/models/config.cairo [324-329]](https://github.com/BibliothecaDAO/eternum/pull/1040/files#diff-e91086e1cdfc697fffe91cd62d9f8ae6882c51f6b3296b60d91a92c47c13b61eR324-R329) ```diff #[derive(Copy, Drop, Serde)] #[dojo::model] +/// Represents the population configuration specific to guilds. struct GuildPopulationConfig { #[key] - config_id: u128, - base_population: u32, + config_id: u128, // Unique identifier for the guild population configuration. + base_population: u32, // Base population limit for a guild. } ``` Suggestion importance[1-10]: 7 Why: Adding a docstring improves code readability and maintainability, but it is not crucial for functionality.	7