BibliothecaDAO / eternum

onchain eternal game
https://alpha-eternum.realms.world
MIT License
36 stars 30 forks source link

mint resources and claim quest in 1 multicall #1053

Closed aymericdelab closed 2 days ago

aymericdelab commented 3 days ago

User description

creates multicall to avoid one tx going through but not the other


PR Type

Enhancement


Description


Changes walkthrough ๐Ÿ“

Relevant files
Enhancement
createSystemCalls.ts
Add mint_resources_and_claim_quest function for combined operations

client/src/dojo/createSystemCalls.ts
  • Added mint_resources_and_claim_quest function to handle minting
    resources and claiming quests in a single call.
  • Updated export list to include mint_resources_and_claim_quest.
  • +8/-2     
    HintBox.tsx
    Use mint_resources_and_claim_quest in HintBox component   

    client/src/ui/components/hints/HintBox.tsx
  • Replaced separate calls to mint_starting_resources and mint_resources
    with a single call to mint_resources_and_claim_quest.
  • +3/-7     
    index.ts
    Add mint_resources_and_claim_quest method to EternumProvider

    sdk/packages/eternum/src/provider/index.ts
  • Added mint_resources_and_claim_quest method to EternumProvider class.
  • +19/-0   
    provider.ts
    Add MintResourcesAndClaimProps interface                                 

    sdk/packages/eternum/src/types/provider.ts
  • Added MintResourcesAndClaimProps interface to define properties for
    combined mint and claim operation.
  • +6/-0     

    ๐Ÿ’ก PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    vercel[bot] commented 3 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 30, 2024 0:23am
    github-actions[bot] commented 3 days ago

    PR Reviewer Guide ๐Ÿ”

    โฑ๏ธ Estimated effort to review [1-5] 3
    ๐Ÿงช Relevant tests No
    ๐Ÿ”’ Security concerns No
    โšก Key issues to review Possible Bug:
    The mint_resources_and_claim_quest function in HintBox.tsx uses entityId directly without checking if it's provided or defaulting to BigInt(0) as the previous implementation did. This might lead to issues if entityId is not provided.
    Efficiency Concern:
    The mint_resources_and_claim_quest function in EternumProvider class constructs a transaction with potentially many calls to mint_starting_resources based on config_ids. This could lead to gas inefficiencies or transaction failures if config_ids array is large.
    github-actions[bot] commented 3 days ago

    PR Code Suggestions โœจ

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Ensure correct handling of resource division in the mint_resources_and_claim_quest method ___ **Modify the mint_resources_and_claim_quest method to ensure that the resources.length / 2
    calculation is correctly handled. This division might lead to unexpected results if
    resources.length is an odd number. Consider validating or adjusting the logic to handle
    such cases.** [sdk/packages/eternum/src/provider/index.ts [139-150]](https://github.com/BibliothecaDAO/eternum/pull/1053/files#diff-467989f145beaf5796d645b8891d6c2468a39d539d62b48a1459185c6fe1de55R139-R150) ```diff const calldata = [ { contractAddress: getContractByName(this.manifest, "dev_resource_systems"), entrypoint: "mint", - calldata: [receiver_id, resources.length / 2, ...resources], + calldata: [receiver_id, Math.ceil(resources.length / 2), ...resources], }, ...config_ids.map((configId) => ({ contractAddress: getContractByName(this.manifest, "realm_systems"), entrypoint: "mint_starting_resources", calldata: [configId, receiver_id], })), ]; ```
    Suggestion importance[1-10]: 9 Why: Correctly handling the division of resources is crucial to avoid unexpected results, especially when dealing with odd numbers. This suggestion addresses a potential bug and improves the accuracy of the function.
    9
    Enhancement
    Add error handling to the mint_resources_and_claim_quests function ___ **Consider adding error handling within the mint_resources_and_claim_quest function to
    manage potential failures in the mint_resources_and_claim_quest method call. This will
    improve the robustness of the function by allowing it to gracefully handle errors and
    provide feedback to the caller.** [client/src/dojo/createSystemCalls.ts [187-189]](https://github.com/BibliothecaDAO/eternum/pull/1053/files#diff-322af6c204272be73084828a2b1bab251422314ba594744850c8659eed13a8faR187-R189) ```diff const mint_resources_and_claim_quest = async (props: SystemProps.MintResourcesAndClaimProps) => { - await provider.mint_resources_and_claim_quest(props); + try { + await provider.mint_resources_and_claim_quest(props); + } catch (error) { + console.error('Failed to mint resources and claim quest:', error); + throw error; // or handle error as appropriate + } }; ```
    Suggestion importance[1-10]: 8 Why: Adding error handling improves the robustness of the function by allowing it to gracefully handle errors and provide feedback to the caller. This is a significant enhancement for reliability.
    8
    Possible issue
    Handle potential undefined values for entityId in the mint_resources_and_claim_quest call ___ **Update the mint_resources_and_claim_quest call to handle potential undefined values for
    entityId. This will prevent runtime errors when entityId is not provided or is null.** [client/src/ui/components/hints/HintBox.tsx [32-37]](https://github.com/BibliothecaDAO/eternum/pull/1053/files#diff-5cf22aac8f0607d764110992d7cb5eac12ced82af11478c86e79fd3659cefc6aR32-R37) ```diff await mint_resources_and_claim_quest({ signer: account, config_ids: quest.prizes.map((prize) => BigInt(prize.id)), - receiver_id: entityId, + receiver_id: entityId || BigInt(0), // Ensure receiver_id is never undefined resources: resourcesToMint, }); ```
    Suggestion importance[1-10]: 7 Why: Handling undefined values for `entityId` prevents runtime errors, enhancing the robustness of the function. This is a useful improvement for preventing potential issues.
    7
    Maintainability
    Refactor the mint_resources_and_claim_quest method to separate concerns for clarity ___ **Refactor the mint_resources_and_claim_quest method to separate the concerns of minting
    resources and claiming quests into distinct steps within the method. This improves
    readability and maintainability by clearly delineating the responsibilities of each part
    of the method.** [sdk/packages/eternum/src/provider/index.ts [139-150]](https://github.com/BibliothecaDAO/eternum/pull/1053/files#diff-467989f145beaf5796d645b8891d6c2468a39d539d62b48a1459185c6fe1de55R139-R150) ```diff -const calldata = [ - { - contractAddress: getContractByName(this.manifest, "dev_resource_systems"), - entrypoint: "mint", - calldata: [receiver_id, resources.length / 2, ...resources], - }, - ...config_ids.map((configId) => ({ - contractAddress: getContractByName(this.manifest, "realm_systems"), - entrypoint: "mint_starting_resources", - calldata: [configId, receiver_id], - })), -]; +const resourceCalldata = { + contractAddress: getContractByName(this.manifest, "dev_resource_systems"), + entrypoint: "mint", + calldata: [receiver_id, Math.ceil(resources.length / 2), ...resources], +}; +const questCalldata = config_ids.map((configId) => ({ + contractAddress: getContractByName(this.manifest, "realm_systems"), + entrypoint: "mint_starting_resources", + calldata: [configId, receiver_id], +})); +const calldata = [resourceCalldata, ...questCalldata]; ```
    Suggestion importance[1-10]: 6 Why: Separating the concerns of minting resources and claiming quests improves readability and maintainability. While this is a good practice, it is not as critical as fixing bugs or adding error handling.
    6