contracts & client config: scale building cost #1722
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Failed to generate code suggestions for PR |
![mentatbot[bot]](https://avatars.githubusercontent.com/in/837875?s=60&v=4){kind=small}
There was a problem hiding this comment.
The implementation of scaled building costs looks good and covers both contract and client-side changes. The new BuildingGeneralConfig model and its usage are well-implemented. The changes to the create function in BuildingCustomImpl to return both the Building and BuildingQuantityv2 are appropriate. The client-side SDK updates in EternumProvider and config/index.ts are consistent with the contract changes. The removal of the BonusPercentageImpl in favor of using PercentageImpl and PercentageValueImpl from the math utils is a good refactoring. The changes seem to be backwards compatible and shouldn't break existing functionality. Overall, this is a solid implementation of the scaled building costs feature.
Thanks for using MentatBot. Give comments a 👍 or 👎 to help me improve!
| @@ -102,7 +102,7 @@ export const ONE_MONTH = 2628000; | |||
|
|
|||
| // Buildings | |||
| export const BASE_POPULATION_CAPACITY = 5; | |||
|
|
|||
| export const BUILDING_FIXED_COST_SCALE_PERCENT = 1_000; // 1_000/10_000 = 10% | |||
There was a problem hiding this comment.
Consider making this scaling factor more easily configurable, possibly as an environment variable or part of a configuration object.
contracts/src/models/buildings.cairo
Outdated
| @@ -598,17 +583,33 @@ impl BuildingCustomImpl of BuildingCustomTrait { | |||
| destroyed_building_category | |||
| } | |||
|
|
|||
| fn make_payment(world: IWorldDispatcher, entity_id: ID, category: BuildingCategory, resource_type: u8) { | |||
| let building_config: BuildingConfig = BuildingConfigCustomImpl::get(world, category, resource_type); | |||
| fn pay_fixed_cost(self: Building, building_quantity: BuildingQuantityv2, world: IWorldDispatcher) { | |||
There was a problem hiding this comment.
can this keep the same name as before? I feel like the function actually pays a variable cost
contracts/src/models/config.cairo
Outdated
| // cost scale percent | ||
| base_cost_percent_increase: u16, |
There was a problem hiding this comment.
remove comment and change the name to be more explicit, we don't want comments to be everywhere if it can be explained through the name imo
There was a problem hiding this comment.
i dont actually know what a better name would be. ive renamed it like 5 times
There was a problem hiding this comment.
increase_per_extra_building_percent ? Honestly I prefer explicit and long than short haha
There was a problem hiding this comment.
quadratic_increase_per_extra_building_percent
There was a problem hiding this comment.
it doesnt fit into a felt. long names wont fit into a felt
There was a problem hiding this comment.
Forgot dojo had those limitations
There was a problem hiding this comment.
31 characters at most
contracts and client config for #1720