Fix/wonders lords cost #2384
Fix/wonders lords cost #2384
Conversation
Landing page UX updates and cartridge controller fix
Sidebar links and ux
This is to Check Tests: DON'T MERGE
fix countdown overlay and bridge out
fix countdown overlay and bridge out
update signing-policy for mainnet (#2379)
Update controller
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
Warning Rate limit exceeded@bob0005 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 20 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe changes in this pull request enhance the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
You are out of MentatBot reviews. Your usage will refresh December 16 at 08:00 AM. |
|
Failed to generate code suggestions for PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
client/src/ui/components/construction/SelectPreviewBuilding.tsx (1)
Line range hint
602-621: Simplify conditional rendering of ongoing costsYou can simplify the conditional rendering by using logical AND (
&&) instead of a ternary operator returningnull. This improves readability.Apply this change:
-{ongoingCost.length > 0 ? ( +{ongoingCost.length > 0 && ( <> <div className="font-bold uppercase">consumed per/s</div> <div className="grid grid-cols-2 gap-2"> {resourceProduced !== 0 && Object.keys(ongoingCost).map((resourceId, index) => { const balance = getBalance(entityId || 0, ongoingCost[Number(resourceId)].resource); return ( <ResourceCost key={`ongoing-cost-${index}`} type="horizontal" resourceId={ongoingCost[Number(resourceId)].resource} amount={ongoingCost[Number(resourceId)].amount} balance={balance.balance} /> ); })} </div> </> - ) : null} +)}
| let ongoingCost = resourceProduced !== undefined ? configManager.resourceInputs[resourceProduced] || [] : []; | ||
|
|
||
| const realm = getComponentValue(dojo.setup.components.Realm, getEntityIdFromKeys([BigInt(entityId || 0)])); | ||
| if (buildingId == BuildingType.Market && realm?.has_wonder && ongoingCost.length > 0) { | ||
| ongoingCost = ongoingCost.map((cost) => | ||
| cost.resource === ResourcesIds.Lords ? { ...cost, amount: cost.amount * 0.1 } : cost, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use the helper function for consistent cost adjustment
Apply the adjustLordsCost helper function here as well to maintain consistency and reduce code duplication.
Update the ongoing cost adjustment:
if (buildingId == BuildingType.Market && realm?.has_wonder && ongoingCost.length > 0) {
- ongoingCost = ongoingCost.map((cost) =>
- cost.resource === ResourcesIds.Lords ? { ...cost, amount: cost.amount * 0.1 } : cost,
- );
+ ongoingCost = adjustLordsCost(ongoingCost);
}Committable suggestion skipped: line range outside the PR's diff.
| let cost = configManager.resourceInputs[resourceId]; | ||
|
|
||
| const realm = getComponentValue(dojo.setup.components.Realm, getEntityIdFromKeys([BigInt(entityId || 0)])); | ||
| if (resourceId == ResourcesIds.Donkey && realm?.has_wonder) { | ||
| cost = cost.map((cost) => (cost.resource === ResourcesIds.Lords ? { ...cost, amount: cost.amount * 0.1 } : cost)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicate cost adjustment logic into a helper function
The logic for adjusting the Lords resource cost when the realm has a wonder is duplicated in lines 402-404 and 539-543. Consider extracting this logic into a reusable helper function to improve maintainability and reduce code duplication.
Apply the following changes:
- Create a helper function
adjustLordsCost:
+const adjustLordsCost = (cost: ResourceCostType[]): ResourceCostType[] => {
+ return cost.map((item) =>
+ item.resource === ResourcesIds.Lords ? { ...item, amount: item.amount * 0.1 } : item,
+ );
+};- Update the cost adjustment in lines 402-404:
if (resourceId == ResourcesIds.Donkey && realm?.has_wonder) {
- cost = cost.map((cost) => (cost.resource === ResourcesIds.Lords ? { ...cost, amount: cost.amount * 0.1 } : cost));
+ cost = adjustLordsCost(cost);
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
sdk/packages/eternum/src/provider/index.ts (1)
306-306: Enhance debug logging for better traceability.The current console.log could be more informative by including additional context about the operation being performed.
- console.log({ resources }); + console.log('Starting withdrawal from realm:', { resources, through_bank_id, from_realm_entity_id });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
.knip.json(2 hunks)client/index.html(0 hunks)client/src/dojo/modelManager/ResourceManager.ts(1 hunks)client/src/ui/components/construction/SelectPreviewBuilding.tsx(4 hunks)client/src/ui/utils/utils.tsx(2 hunks)common/addresses/addresses.MAINNET.json(1 hunks)config/index.ts(1 hunks)contracts/.tool-versions(1 hunks)landing/public/resource_addresses/mainnet/resource_addresses.json(1 hunks)landing/src/components/modules/bridge-in.tsx(2 hunks)landing/src/components/modules/bridge-out-step-1.tsx(1 hunks)landing/src/components/modules/bridge-out-step-2.tsx(4 hunks)landing/src/components/modules/season-start-timer.tsx(1 hunks)landing/src/components/ui/elements/CountdownTimer.tsx(1 hunks)landing/src/hooks/useBridge.tsx(1 hunks)landing/src/routeTree.gen.ts(2 hunks)sdk/packages/eternum/src/provider/index.ts(1 hunks)
💤 Files with no reviewable changes (1)
- client/index.html
✅ Files skipped from review due to trivial changes (9)
- contracts/.tool-versions
- landing/public/resource_addresses/mainnet/resource_addresses.json
- common/addresses/addresses.MAINNET.json
- landing/src/components/modules/season-start-timer.tsx
- landing/src/components/modules/bridge-out-step-1.tsx
- landing/src/hooks/useBridge.tsx
- landing/src/components/ui/elements/CountdownTimer.tsx
- client/src/dojo/modelManager/ResourceManager.ts
- landing/src/routeTree.gen.ts
🔇 Additional comments (6)
.knip.json (1)
15-16: LGTM: Configuration updates for ignored paths.
The additions to the ignore list are appropriate for excluding documentation and configuration files from processing.
Also applies to: 29-32
client/src/ui/utils/utils.tsx (1)
499-501: Well-implemented helper function!
The adjustWonderLordsCost function is a clean, pure function that handles a single responsibility well. It correctly adjusts the Lords resource cost while maintaining immutability.
client/src/ui/components/construction/SelectPreviewBuilding.tsx (2)
400-405: Good implementation of wonder cost adjustment!
The code correctly applies the wonder cost adjustment for donkeys while maintaining clean variable scoping.
537-543: Good implementation of market building cost adjustment!
The code correctly applies the wonder cost adjustment for the market building, following the same pattern as the resource cost adjustment.
landing/src/components/modules/bridge-in.tsx (1)
250-250: Clean formatting improvement!
The removal of the unnecessary line break improves code readability while maintaining the same logical condition.
sdk/packages/eternum/src/provider/index.ts (1)
308-312: Verify the contract address resolution for resource bridge systems.
The contract address resolution is critical for this financial operation. Let's verify that the contract name resolution is consistent across the codebase.
✅ Verification successful
The search results show consistent usage of the contract name ${NAMESPACE}-resource_bridge_systems across the codebase:
- In the SDK (
sdk/packages/eternum/src/provider/index.ts), it's used withgetContractByNamefor various operations like deposit, withdraw, etc. - In deployment scripts (
season_resources/scripts/deployment/libs/commands.js), it follows the same pattern - In the contracts (
contracts/src/systems/resources/contracts/resource_bridge_systems.cairo), it's properly defined and tested - In manifest files (
contracts/manifest_*.json), it's consistently registered with the tags0_eternum-resource_bridge_systems
The contract name resolution in the code under review matches the pattern used throughout the codebase and is correctly implemented.
Contract name resolution for resource bridge systems is correctly implemented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify contract name usage consistency
# Test: Search for resource bridge systems contract references
rg -A 2 "resource_bridge_systems"
Length of output: 31726
| (setupConfig.bridge = { | ||
| ...EternumGlobalConfig.bridge, | ||
| velords_fee_recipient: BigInt('0x045c587318c9ebcf2fbe21febf288ee2e3597a21cd48676005a5770a50d433c5'), // burner | ||
| velords_fee_recipient: BigInt("0x045c587318c9ebcf2fbe21febf288ee2e3597a21cd48676005a5770a50d433c5"), // burner | ||
| season_pool_fee_recipient: BigInt(getContractByName(manifest, `${NAMESPACE}-season_systems`)), | ||
| }, | ||
|
|
||
| // Season Pass | ||
| setupConfig.season = { | ||
| ...EternumGlobalConfig.season, | ||
| seasonPassAddress: process.env.VITE_SEASON_PASS_ADDRESS!, | ||
| realmsAddress: process.env.VITE_REALMS_ADDRESS!, | ||
| lordsAddress: process.env.VITE_LORDS_ADDRESS!, | ||
| }; | ||
| }), |
There was a problem hiding this comment.
Avoid hardcoding wallet addresses in production code.
The bridge configuration uses a hardcoded burner address for velords_fee_recipient. This could be risky in production and makes the code less maintainable.
Consider using an environment variable instead:
- velords_fee_recipient: BigInt("0x045c587318c9ebcf2fbe21febf288ee2e3597a21cd48676005a5770a50d433c5"), // burner
+ velords_fee_recipient: BigInt(process.env.VITE_VELORDS_FEE_RECIPIENT!),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (setupConfig.bridge = { | |
| ...EternumGlobalConfig.bridge, | |
| velords_fee_recipient: BigInt('0x045c587318c9ebcf2fbe21febf288ee2e3597a21cd48676005a5770a50d433c5'), // burner | |
| velords_fee_recipient: BigInt("0x045c587318c9ebcf2fbe21febf288ee2e3597a21cd48676005a5770a50d433c5"), // burner | |
| season_pool_fee_recipient: BigInt(getContractByName(manifest, `${NAMESPACE}-season_systems`)), | |
| }, | |
| // Season Pass | |
| setupConfig.season = { | |
| ...EternumGlobalConfig.season, | |
| seasonPassAddress: process.env.VITE_SEASON_PASS_ADDRESS!, | |
| realmsAddress: process.env.VITE_REALMS_ADDRESS!, | |
| lordsAddress: process.env.VITE_LORDS_ADDRESS!, | |
| }; | |
| }), | |
| (setupConfig.bridge = { | |
| ...EternumGlobalConfig.bridge, | |
| velords_fee_recipient: BigInt(process.env.VITE_VELORDS_FEE_RECIPIENT!), | |
| season_pool_fee_recipient: BigInt(getContractByName(manifest, `${NAMESPACE}-season_systems`)), | |
| }), |
| (setupConfig.season = { | ||
| ...EternumGlobalConfig.season, | ||
| seasonPassAddress: process.env.VITE_SEASON_PASS_ADDRESS!, | ||
| realmsAddress: process.env.VITE_REALMS_ADDRESS!, | ||
| lordsAddress: process.env.VITE_LORDS_ADDRESS!, | ||
| }); |
There was a problem hiding this comment.
💡 Codebase verification
Environment variables are validated but documentation needs improvement
The environment variables are properly validated using Zod schema in both client/env.ts and landing/env.ts:
VITE_SEASON_PASS_ADDRESS: z.string().startsWith("0x"),
VITE_REALMS_ADDRESS: z.string().startsWith("0x"),
VITE_LORDS_ADDRESS: z.string().startsWith("0x"),However, there are a few concerns:
- The variables are populated through deployment scripts (
scripts/deploy.shandscripts/indexer.sh) - No sample
.envfile or documentation found to guide developers on setting up these required variables - The use of non-null assertion (
!) in the config suggests these variables are required but this isn't documented
🔗 Analysis chain
Verify environment variables for season configuration.
The season configuration relies on multiple environment variables without validation.
Let's verify these environment variables are properly documented:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if these environment variables are documented
echo "Checking for environment variables documentation..."
rg -l "VITE_SEASON_PASS_ADDRESS|VITE_REALMS_ADDRESS|VITE_LORDS_ADDRESS" .env* README.md docs/
Length of output: 306
Script:
#!/bin/bash
# Let's first find any .env files and documentation files in the repository
echo "Finding environment and documentation files..."
fd -t f "^\.env" -t f "^README" -t f "\.md$"
# Let's also check for environment variable usage in the codebase
echo -e "\nChecking environment variable usage..."
rg "VITE_(SEASON_PASS|REALMS|LORDS)_ADDRESS"
# Let's check if there's any validation logic for these variables
echo -e "\nChecking for validation logic..."
rg "process\.env\.VITE_(SEASON_PASS|REALMS|LORDS)_ADDRESS" -A 2 -B 2
Length of output: 5352
| setSelectedResourceIds( | ||
| currentDonkeyInfo!.donkeyResources.map((resource) => resource.resourceId as never) ?? 0, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and error handling in resource selection.
The current implementation has several issues:
- Unsafe null assertion with
currentDonkeyInfo! - Type casting with
as neveris a code smell - Missing error handling for edge cases
Consider this safer implementation:
- setSelectedResourceIds(
- currentDonkeyInfo!.donkeyResources.map((resource) => resource.resourceId as never) ?? 0,
- );
+ if (currentDonkeyInfo?.donkeyResources) {
+ setSelectedResourceIds(
+ currentDonkeyInfo.donkeyResources.map((resource) => resource.resourceId)
+ );
+ } else {
+ setSelectedResourceIds([]);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| setSelectedResourceIds( | |
| currentDonkeyInfo!.donkeyResources.map((resource) => resource.resourceId as never) ?? 0, | |
| ); | |
| if (currentDonkeyInfo?.donkeyResources) { | |
| setSelectedResourceIds( | |
| currentDonkeyInfo.donkeyResources.map((resource) => resource.resourceId) | |
| ); | |
| } else { | |
| setSelectedResourceIds([]); | |
| } |
|
|
||
| return await this.executeAndCheckTransaction(signer, [ | ||
| { | ||
| contractAddress: getContractByName(this.manifest, `${NAMESPACE}-resource_bridge_systems`), | ||
| entrypoint: "finish_withdraw", | ||
| calldata: [through_bank_id, from_entity_id, tokenAddress, recipient_address, client_fee_recipient], | ||
| }, | ||
| ]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for invalid addresses.
While the formatting improvements look good, this critical financial operation should include validation for the input addresses.
public async bridge_finish_withdraw_from_realm(props: SystemProps.BridgeFinishWithdrawFromRealmProps) {
const { tokenAddress, through_bank_id, from_entity_id, recipient_address, client_fee_recipient, signer } = props;
+
+ // Validate addresses
+ if (!tokenAddress || !recipient_address || !client_fee_recipient) {
+ throw new Error('Invalid addresses provided for bridge withdrawal completion');
+ }
return await this.executeAndCheckTransaction(signer, [
{
contractAddress: getContractByName(this.manifest, `${NAMESPACE}-resource_bridge_systems`),
entrypoint: "finish_withdraw",
calldata: [through_bank_id, from_entity_id, tokenAddress, recipient_address, client_fee_recipient],
},
]);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return await this.executeAndCheckTransaction(signer, [ | |
| { | |
| contractAddress: getContractByName(this.manifest, `${NAMESPACE}-resource_bridge_systems`), | |
| entrypoint: "finish_withdraw", | |
| calldata: [through_bank_id, from_entity_id, tokenAddress, recipient_address, client_fee_recipient], | |
| }, | |
| ]); | |
| public async bridge_finish_withdraw_from_realm(props: SystemProps.BridgeFinishWithdrawFromRealmProps) { | |
| const { tokenAddress, through_bank_id, from_entity_id, recipient_address, client_fee_recipient, signer } = props; | |
| // Validate addresses | |
| if (!tokenAddress || !recipient_address || !client_fee_recipient) { | |
| throw new Error('Invalid addresses provided for bridge withdrawal completion'); | |
| } | |
| return await this.executeAndCheckTransaction(signer, [ | |
| { | |
| contractAddress: getContractByName(this.manifest, `${NAMESPACE}-resource_bridge_systems`), | |
| entrypoint: "finish_withdraw", | |
| calldata: [through_bank_id, from_entity_id, tokenAddress, recipient_address, client_fee_recipient], | |
| }, | |
| ]); | |
| } |
Closes #2354
Summary by CodeRabbit