Closed edisontim closed 6 days ago
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 26, 2024 1:32pm |
β±οΈ Estimated effort to review [1-5] | 3 |
π§ͺ Relevant tests | No |
π Security concerns | No |
β‘ Key issues to review |
Possible Bug: The refactoring from a single config_id to config_ids array in the mint_starting_resources function might cause issues if not all calling functions have been updated to pass an array instead of a single value. |
Performance Concern: The change to multicall in mint_starting_resources could lead to increased gas costs or unexpected behavior if not handled properly in all scenarios. |
Category | Suggestion | Score |
Best practice |
Enhance clarity by using parameter destructuring directly in the function signature___ **Use destructuring directly in the function parameter to enhance code clarity and reduceredundancy.** [sdk/packages/eternum/src/provider/index.ts [542-553]](https://github.com/BibliothecaDAO/eternum/pull/1025/files#diff-467989f145beaf5796d645b8891d6c2468a39d539d62b48a1459185c6fe1de55R542-R553) ```diff -public async mint_starting_resources(props: SystemProps.MintStartingResources) { - const { realm_entity_id, config_ids, signer } = props; +public async mint_starting_resources({ realm_entity_id, config_ids, signer }: SystemProps.MintStartingResources) { return await this.executeAndCheckTransaction( signer, config_ids.map((configId) => ({ contractAddress: getContractByName(this.manifest, "realm_systems"), entrypoint: "mint_starting_resources", calldata: [configId, realm_entity_id], }), ); } ``` Suggestion importance[1-10]: 9Why: Using destructuring directly in the function parameter enhances code clarity and reduces redundancy, making the function signature cleaner and more readable. | 9 |
Maintainability |
Simplify control flow by refactoring nested try-catch blocks into a single block___ **Refactor the nested try-catch blocks inhandleAllClaims to a single try-catch block to simplify the control flow and improve readability.** [client/src/ui/components/hints/HintBox.tsx [28-38]](https://github.com/BibliothecaDAO/eternum/pull/1025/files#diff-5cf22aac8f0607d764110992d7cb5eac12ced82af11478c86e79fd3659cefc6aR28-R38) ```diff try { - try { - await mint_starting_resources({ - signer: account, - config_ids: quest.prizes.map((prize) => BigInt(prize.id)), - realm_entity_id: entityId || "0", - }); - } catch (error) { - console.error(`Failed to claim resources for quest ${quest.name}:`, error); - } + await mint_starting_resources({ + signer: account, + config_ids: quest.prizes.map((prize) => BigInt(prize.id)), + realm_entity_id: entityId || "0", + }); } catch (error) { - console.error("Failed to claim resources:", error); + console.error(`Failed to claim resources:`, error); } ``` Suggestion importance[1-10]: 8Why: The suggestion to refactor the nested try-catch blocks into a single block improves readability and simplifies the control flow, making the code easier to maintain. | 8 |
Enhancement |
Improve naming consistency and clarity by renaming the interface___ **Rename theMintStartingResources interface to MintResourcesProps to better reflect its usage and align with naming conventions.** [sdk/packages/eternum/src/types/provider.ts [322-325]](https://github.com/BibliothecaDAO/eternum/pull/1025/files#diff-3d3d05c5e7f20c103bd35e9560b88dc343f73afc5b5116cd75f3d9c382bfe574R322-R325) ```diff -export interface MintStartingResources extends SystemSigner { +export interface MintResourcesProps extends SystemSigner { config_ids: num.BigNumberish[]; realm_entity_id: num.BigNumberish; } ``` Suggestion importance[1-10]: 7Why: Renaming the interface to `MintResourcesProps` improves naming consistency and clarity, aligning better with common naming conventions. However, this change is more of an enhancement rather than a critical improvement. | 7 |
User description
Closes #1019
PR Type
Enhancement
Description
CreateStartingResources
toMintStartingResources
across multiple files for consistency.config_ids
to be processed in a single transaction.handleClaimResources
function and updatedhandleAllClaims
to streamline the resource claiming process.HintBox.tsx
for better readability.Changes walkthrough π
createSystemCalls.ts
Rename
CreateStartingResources
toMintStartingResources
in systemcalls
client/src/dojo/createSystemCalls.ts
CreateStartingResources
toMintStartingResources
in themint_starting_resources
function.HintBox.tsx
Refactor quest resource claiming to use multicall
client/src/ui/components/hints/HintBox.tsx
handleClaimResources
function.handleAllClaims
to usemint_starting_resources
with multipleconfig_ids
.index.ts
Enhance `mint_starting_resources` to support multicall
sdk/packages/eternum/src/provider/index.ts
mint_starting_resources
to handle multipleconfig_ids
in asingle transaction.
provider.ts
Update
MintStartingResources
interface to support multiple config IDssdk/packages/eternum/src/types/provider.ts
CreateStartingResources
interface toMintStartingResources
.config_id
toconfig_ids
as an array in the interface.