Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Domain ID value not correctly recovered from chain data #694

Conversation

marc-aurele-besner
Copy link
Collaborator

@marc-aurele-besner marc-aurele-besner commented Jul 8, 2024

User description

Domain ID value not correctly recovered from chain data

Causing every option to have the domain id 0


PR Type

Bug fix, Enhancement


Description

  • Fixed the domainId assignment in the dropdown mapping function to use the index instead of an incorrect primitive value.
  • Bumped the storage version to 2 for domain states.

Changes walkthrough 📝

Relevant files
Bug fix
useDomainsData.ts
Fix domain ID assignment in dropdown mapping function       

explorer/src/hooks/useDomainsData.ts

  • Fixed the domainId assignment to use the index instead of incorrect
    primitive value.
  • Adjusted the mapping function to include the index parameter.
  • +2/-2     
    Enhancement
    domains.ts
    Update storage version to 2                                                           

    explorer/src/states/domains.ts

    • Bumped the storage version to 2.
    +1/-0     

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

    Copy link

    netlify bot commented Jul 8, 2024

    Deploy Preview for dev-astral ready!

    Name Link
    🔨 Latest commit 4f01143
    🔍 Latest deploy log https://app.netlify.com/sites/dev-astral/deploys/668c35456c1cd40008ff15d5
    😎 Deploy Preview https://deploy-preview-694--dev-astral.netlify.app
    📱 Preview on mobile
    Toggle QR Code...

    QR Code

    Use your smartphone camera to open QR code link.

    To edit notification comments on pull requests, go to your Netlify site configuration.

    Copy link

    github-actions bot commented Jul 8, 2024

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Possible Bug:
    The change in useDomainsData.ts replaces the domainId assignment from a primitive value to an index. This might not be correct if the order of domains in domainRegistry is not guaranteed to match their actual IDs. This could lead to incorrect domain IDs being displayed or used.

    @EmilFattakhov
    Copy link
    Contributor

    That was fast! Waiting for the test build to built.

    Copy link

    github-actions bot commented Jul 8, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Replace index-based domain identification with a stable, data-derived identifier

    Replace the use of index.toString() for domainId with a more reliable identifier from the
    domain object itself. Using the index as an identifier can lead to issues if the order of
    domains changes, which might not reflect the actual identity of the domain.

    explorer/src/hooks/useDomainsData.ts [22]

    -domainId: index.toString(),
    +domainId: domain[0].toPrimitive() as string,
     
    Suggestion importance[1-10]: 9

    Why: Using the index as an identifier can lead to issues if the order of domains changes. A stable identifier from the domain object itself is more reliable and prevents potential bugs related to domain identity.

    9
    Best practice
    Ensure data migration strategies are in place when updating storage version numbers

    When updating the version number in a state management setup, ensure that migration
    strategies or handlers are in place to manage the transition of stored data from the old
    version to the new version. This is crucial to prevent data loss or inconsistencies.

    explorer/src/states/domains.ts [31]

    -version: 2,
    +version: 2, // Ensure to implement migration strategies for version changes
     
    Suggestion importance[1-10]: 8

    Why: Updating the version number without implementing migration strategies can lead to data loss or inconsistencies. This suggestion highlights a crucial best practice for state management.

    8

    @EmilFattakhov
    Copy link
    Contributor

    Screenshot 2024-07-08 at 2 44 07 PM

    Something still seems a bit off @marc-aurele-besner

    Copy link
    Contributor

    @EmilFattakhov EmilFattakhov left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Thanks Marc, seem to work well!

    @marc-aurele-besner marc-aurele-besner merged commit 855ac77 into main Jul 8, 2024
    13 checks passed
    @marc-aurele-besner marc-aurele-besner deleted the 693-selecting-domain-to-become-an-operator-on-is-not-functioning-properly branch July 8, 2024 19:16
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    Selecting domain to become an Operator on is not functioning properly
    3 participants