BibliothecaDAO / eternum

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

Quest resources #1046

Closed aymericdelab closed 4 days ago

aymericdelab commented 4 days ago

PR Type

Enhancement, Bug fix


Description


Changes walkthrough ๐Ÿ“

Relevant files
Enhancement
createSystemCalls.ts
Add mint_resources function to system calls                           

client/src/dojo/createSystemCalls.ts
  • Added mint_resources function to mint resources.
  • Included mint_resources in the exported system calls.
  • +5/-0     
    useRealm.tsx
    Add getQuestResources function to useRealm hook                   

    client/src/hooks/helpers/useRealm.tsx
  • Imported getQuestResources from @bibliothecadao/eternum.
  • Added getQuestResources function to fetch quest resources.
  • Included getQuestResources in the returned object of useRealm.
  • +10/-0   
    HintBox.tsx
    Integrate dynamic quest resources in HintBox component     

    client/src/ui/components/hints/HintBox.tsx
  • Added getQuestResources usage in HintBox component.
  • Updated handleAllClaims to use mint_resources.
  • Modified QuestRewards to use dynamic quest resources.
  • +35/-17 
    index.ts
    Remove setQuestConfig function from configuration               

    sdk/packages/eternum/src/config/index.ts - Removed `setQuestConfig` function.
    +0/-19   
    global.ts
    Add startingResourcesInputProductionFactor to global config

    sdk/packages/eternum/src/constants/global.ts
  • Added startingResourcesInputProductionFactor to global configuration.
  • +1/-0     
    resources.ts
    Update resource amounts for quest types                                   

    sdk/packages/eternum/src/constants/resources.ts - Adjusted resource amounts for various quest types.
    +30/-30 
    index.ts
    Add utility functions and implement getQuestResources       

    sdk/packages/eternum/src/utils/index.ts
  • Added utility functions for resource inputs and production factors.
  • Implemented getQuestResources function.
  • Removed QUEST_RESOURCES_SCALED export.
  • +46/-4   

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

    vercel[bot] commented 4 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 29, 2024 11:06am
    github-actions[bot] commented 4 days ago

    PR Reviewer Guide ๐Ÿ”

    โฑ๏ธ Estimated effort to review [1-5] 4
    ๐Ÿงช Relevant tests No
    ๐Ÿ”’ Security concerns No
    โšก Key issues to review Possible Bug:
    The getQuestResources function in useRealm.tsx uses useRealmStore inside a regular function which might lead to unexpected behaviors as hooks should only be called at the top level of React functional components or custom hooks.
    Performance Concern:
    The handleAllClaims function in HintBox.tsx might lead to performance issues due to multiple state updates and potentially large and nested data manipulations with flatMap and map inside a loop.
    Code Clarity:
    The getQuestResources function in useRealm.tsx could be refactored for better clarity and efficiency. The current implementation with nested access and transformations is hard to read and maintain.
    github-actions[bot] commented 4 days ago

    PR Code Suggestions โœจ

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Manage loading state effectively in handleAllClaims ___ **Implement loading state management in handleAllClaims to disable interaction or provide
    user feedback during the asynchronous operations. This prevents multiple submissions and
    improves user experience.** [client/src/ui/components/hints/HintBox.tsx [23-44]](https://github.com/BibliothecaDAO/eternum/pull/1046/files#diff-5cf22aac8f0607d764110992d7cb5eac12ced82af11478c86e79fd3659cefc6aR23-R44) ```diff const handleAllClaims = async () => { - const questResources = getQuestResources(); - const resourcesToMint = quest.prizes.flatMap((prize) => { - const resources = questResources[prize.id]; - return resources.flatMap((resource) => [resource.resource as number, multiplyByPrecision(resource.amount)]); - }); setIsLoading(true); try { + const questResources = getQuestResources(); + const resourcesToMint = quest.prizes.flatMap((prize) => { + const resources = questResources[prize.id]; + return resources.flatMap((resource) => [resource.resource as number, multiplyByPrecision(resource.amount)]); + }); await mint_starting_resources({ signer: account, config_ids: quest.prizes.map((prize) => BigInt(prize.id)), realm_entity_id: entityId || BigInt(0), }); await mint_resources({ signer: account, receiver_id: entityId || BigInt(0), resources: resourcesToMint, }); } catch (error) { console.error(`Failed to claim resources for quest ${quest.name}:`, error); + } finally { + setIsLoading(false); } }; ```
    Suggestion importance[1-10]: 9 Why: Implementing loading state management is important for preventing multiple submissions and improving user experience. This enhancement is highly beneficial for the application's usability.
    9
    Possible issue
    Add error handling to the mint_resources function ___ **Consider adding error handling for the mint_resources function to manage potential
    failures during the resource minting process. This can help in debugging and maintaining
    proper flow control, especially in production environments.** [client/src/dojo/createSystemCalls.ts [183-185]](https://github.com/BibliothecaDAO/eternum/pull/1046/files#diff-322af6c204272be73084828a2b1bab251422314ba594744850c8659eed13a8faR183-R185) ```diff const mint_resources = async (props: SystemProps.MintResourcesProps) => { - await provider.mint_resources(props); + try { + await provider.mint_resources(props); + } catch (error) { + console.error('Failed to mint resources:', error); + throw error; // Rethrow or handle as needed + } }; ```
    Suggestion importance[1-10]: 8 Why: Adding error handling is crucial for debugging and maintaining proper flow control, especially in production environments. This suggestion addresses a potential issue effectively.
    8
    Performance
    Use a Set to collect unique resources in uniqueResourceInputs ___ **Optimize the uniqueResourceInputs function by using a Set for collecting unique resources,
    which can improve performance by avoiding the need for manual checking of array inclusion.** [sdk/packages/eternum/src/utils/index.ts [28-39]](https://github.com/BibliothecaDAO/eternum/pull/1046/files#diff-400ba73026f7862b3b875705fe5d75aa03c455d04bc54c861294baccb84e4dd6R28-R39) ```diff export const uniqueResourceInputs = (resourcesProduced: number[]): number[] => { - let uniqueResourceInputs: number[] = []; + const uniqueResourceInputs = new Set(); for (let resourceProduced of resourcesProduced) { for (let resourceInput of RESOURCE_INPUTS[resourceProduced]) { - if (!uniqueResourceInputs.includes(resourceInput.resource)) { - uniqueResourceInputs.push(resourceInput.resource); - } + uniqueResourceInputs.add(resourceInput.resource); } } - return uniqueResourceInputs; + return Array.from(uniqueResourceInputs); }; ```
    Suggestion importance[1-10]: 8 Why: Using a `Set` to collect unique resources improves performance by avoiding manual checking of array inclusion. This optimization is effective and enhances the function's efficiency.
    8
    Best practice
    Refactor getQuestResources to avoid direct hook calls within the function ___ **Refactor the getQuestResources function to avoid using hooks (useRealmStore) directly
    inside the function body. This can lead to unexpected behavior and violates React's rules
    of hooks. Instead, pass realmEntityId as a parameter to the function.** [client/src/hooks/helpers/useRealm.tsx [26-30]](https://github.com/BibliothecaDAO/eternum/pull/1046/files#diff-7a3368b90ccd171419e1dd68aafbbf5d20b6e3a231a12d711de4ff05ba9bed0cR26-R30) ```diff -const getQuestResources = () => { - const realmEntityId = useRealmStore.getState().realmEntityId; +const getQuestResources = (realmEntityId) => { const realm = getComponentValue(Realm, getEntityIdFromKeys([BigInt(realmEntityId)])); const resourcesProduced = realm ? unpackResources(realm.resource_types_packed, realm.resource_types_count) : []; return getStartingResources(resourcesProduced); }; ```
    Suggestion importance[1-10]: 7 Why: Refactoring to avoid direct hook calls within the function is a best practice in React, preventing unexpected behavior and adhering to React's rules of hooks. This improves code maintainability and reliability.
    7