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.

Sign up for GitHub

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

Jump to bottom

Wallet sidekick staking summary not accurate #571

Merged
merged 3 commits into from Apr 25, 2024
Merged

Wallet sidekick staking summary not accurate #571

merged 3 commits into from Apr 25, 2024

Conversation

marc-aurele-besner
Copy link
Collaborator

@marc-aurele-besner marc-aurele-besner commented Apr 24, 2024 •
edited

User description

Wallet sidekick staking summary not accurate

  • Filter out nominations from user operator
  • Remove operators from the nominator count

Related issue

Screenshot

WalletSideKick

Type

bug_fix, enhancement

Description

  • Updated the StakingSummary GraphQL query to include operator ownership details, enhancing data accuracy.
  • Adjusted related GraphQL type definitions and functions to align with the updated query structure.
  • Modified the WalletSideKick component to improve the accuracy of staking summary calculations by excluding operators from the nominator count and filtering out nominations from the user's own operator.
  • Enhanced the QUERY_STAKING_SUMMARY to include operatorOwner in the operator details for better data clarity.

Changes walkthrough

Relevant files
Enhancement
gql.ts
Update GraphQL StakingSummary Query and Function                 

explorer/gql/gql.ts

  • Updated the GraphQL query StakingSummary to include operatorOwner in
    the operator details.
  • Adjusted the GraphQL function to use the updated StakingSummary query.

    		•  +2/-2     
    graphql.ts
    Update GraphQL Type Definitions for Staking Summary           

    explorer/gql/graphql.ts

  • Updated the StakingSummaryQuery type definition to reflect changes in
    the GraphQL query.
    		•  +2/-2     
    StakingSummary.tsx
    Refine Staking Summary Calculations in Wallet Component   

    explorer/src/components/WalletSideKick/StakingSummary.tsx

  • Modified the calculation of totalNominatedCount to exclude operators.
  • Updated totalNominatedStake calculation to filter out nominations from
    the user's own operator.
    		•  +6/-2     
    query.ts
    Enhance GraphQL Query for Staking Summary                               

    explorer/src/components/WalletSideKick/query.ts

  • Added operatorOwner to the operator details in the
    QUERY_STAKING_SUMMARY GraphQL query.
    		•  +1/-0     

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

    @marc-aurele-besner marc-aurele-besner linked an issue Apr 24, 2024 that may be closed by this pull request
    Wallet Sidekick - Staking Summary not accurate #550
    Open
    @netlify Netlify
    Copy link

    netlify bot commented Apr 24, 2024 •
    edited

    ✅ Deploy Preview for astral-prod ready!

    Name Link
    🔨 Latest commit 026b649
    🔍 Latest deploy log https://app.netlify.com/sites/astral-prod/deploys/662933406fb28e0008628ba9
    😎 Deploy Preview https://deploy-preview-571--astral-prod.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.

    @netlify Netlify
    Copy link

    netlify bot commented Apr 24, 2024 •
    edited

    ✅ Deploy Preview for storybook-pr canceled.

    Name Link
    🔨 Latest commit 026b649
    🔍 Latest deploy log https://app.netlify.com/sites/storybook-pr/deploys/66293340876f0b0009b29201

    @marc-aurele-besner marc-aurele-besner added the bug Something isn't working label Apr 24, 2024
    @marc-aurele-besner marc-aurele-besner changed the base branch from main to staging April 24, 2024 16:29
    @github-actions github-actions bot added enhancement New feature or request bug_fix labels Apr 24, 2024
    @github-actions GitHub Actions
    Copy link

    PR Description updated to latest commit (026b649)

    @github-actions GitHub Actions
    Copy link

    PR Review

    ⏱️ Estimated effort to review [1-5]

    3, because the PR involves changes to GraphQL queries and TypeScript type definitions, which require careful review to ensure data consistency and correct query functionality. The logic changes in the component also need to be verified for correctness in handling the data.

    🧪 Relevant tests

    No

    🔍 Possible issues

    Possible Bug: The updated StakingSummary query and the corresponding TypeScript type might not handle null values gracefully. If operatorOwner or other fields are null, this could lead to runtime errors in the application.

    🔒 Security concerns

    No

    Code feedback:
    relevant fileexplorer/gql/gql.ts
    suggestion      

    Consider adding error handling for null values in the GraphQL query results to prevent runtime errors. [important]
    relevant line"\n query StakingSummary($first: Int!, $subspaceAccount: String) {\n operators: operatorsConnection(\n orderBy: id_ASC\n first: $first\n where: { operatorOwner_eq: $subspaceAccount }\n ) {\n edges {\n node {\n id\n operatorOwner\n currentDomainId\n currentTotalStake\n totalShares\n }\n }\n totalCount\n }\n nominators: nominatorsConnection(\n orderBy: id_ASC\n first: $first\n where: { account: { id_eq: $subspaceAccount } }\n ) {\n edges {\n node {\n id\n shares\n account {\n id\n }\n operator {\n id\n operatorOwner\n currentDomainId\n currentTotalStake\n totalShares\n }\n }\n }\n totalCount\n }\n }\n"
    relevant fileexplorer/src/components/WalletSideKick/StakingSummary.tsx
    suggestion      

    Ensure that the filtering logic correctly handles cases where operatorOwner might be undefined or null to avoid incorrect calculations. [important]
    relevant line.filter((nominator) => nominator.operator.operatorOwner !== subspaceAccount)
    ✨ Review tool usage guide:

    Overview:
    The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

    The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

    • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
    /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...

    [pr_reviewer]
some_config1=...
some_config2=...

    See the review usage page for a comprehensive guide on using this tool.

    @github-actions GitHub Actions
    Copy link

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Enhancement
    Add a filter to exclude self-owned operators in the StakingSummary query.

    Consider adding a filter to exclude operators owned by the subspaceAccount in the
    StakingSummary query. This ensures that the summary only includes relevant operators,
    enhancing the accuracy of the staking data presented.

    explorer/gql/gql.ts [50]

     operators: operatorsConnection(
   orderBy: id_ASC
   first: $first
-  where: { operatorOwner_eq: $subspaceAccount }
+  where: { operatorOwner_eq: $subspaceAccount, NOT: { id_eq: $subspaceAccount } }
 )
    Include the status field in the operator query to provide more comprehensive data.

    Add a field to fetch the status of the operator in the GraphQL query for
    QUERY_STAKING_SUMMARY. This can provide additional useful information about the operator's
    current state.

    explorer/src/components/WalletSideKick/query.ts [95-100]

     operator {
   id
   operatorOwner
   currentDomainId
   currentTotalStake
   totalShares
+  status
 }
    Bug
    Ensure safe subtraction in totalNominatedCount calculation.

    Modify the calculation of totalNominatedCount to ensure it does not subtract
    totalOperatorCount when nominators is null. This prevents potential errors or incorrect
    calculations when data is not available.

    explorer/src/components/WalletSideKick/StakingSummary.tsx [53-55]

     const totalNominatedCount = useMemo(
-  () => (nominators ? nominators.totalCount - totalOperatorCount : 0),
+  () => nominators ? Math.max(0, nominators.totalCount - totalOperatorCount) : 0,
   [nominators, totalOperatorCount],
 )
    Add null and length checks for nominatorsConnection before processing.

    Add a check to ensure that nominatorsConnection is not null before attempting to filter
    and reduce it, to avoid potential runtime errors.

    explorer/src/components/WalletSideKick/StakingSummary.tsx [57-61]

     const totalNominatedStake = useMemo(
   () =>
-    nominatorsConnection
+    nominatorsConnection && nominatorsConnection.length > 0
       ? nominatorsConnection
         .filter((nominator) => nominator.operator.operatorOwner !== subspaceAccount)
         .reduce(
           (acc, nominator) =>
             acc +
    ✨ Improve tool usage guide:

    Overview:
    The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

    • When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
    /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...

    [pr_code_suggestions]
some_config1=...
some_config2=...

    See the improve usage page for a comprehensive guide on using this tool.

    dnoishi
    dnoishi approved these changes Apr 24, 2024
    View reviewed changes
    Copy link
    Contributor

    @dnoishi dnoishi left a comment

    Choose a reason for hiding this comment

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

    LGTM

    @marc-aurele-besner marc-aurele-besner merged commit 1922fed into staging Apr 25, 2024
    19 checks passed
    @marc-aurele-besner marc-aurele-besner deleted the 550-wallet-sidekick-staking-summary-not-accurate branch April 25, 2024 10:54
    @marc-aurele-besner marc-aurele-besner mentioned this pull request Apr 29, 2024
    Merged
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @dnoishi dnoishi dnoishi approved these changes

    @jfrank-summit jfrank-summit Awaiting requested review from jfrank-summit

    Assignees
    No one assigned
    Labels
    bug_fix bug Something isn't working enhancement New feature or request Review effort [1-5]: 3
    Projects
    Subspace Block Explorer
    Status: Done
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.

    Wallet Sidekick - Staking Summary not accurate
    2 participants
    @marc-aurele-besner @dnoishi