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

Add flow to claim operator staking token #668

Merged
merged 15 commits into from
Jul 3, 2024

Conversation

marc-aurele-besner
Copy link
Collaborator

@marc-aurele-besner marc-aurele-besner commented Jun 25, 2024

User description

Add flow to claim operator staking token

  • Claim token to cover operator minimum stake
  • Add log in fauna db and slack

Screenshot

Before
InProcess
Claimed
ClaimedAlready

slackStats slackLowBalance

PR Type

Enhancement, Documentation


Description

  • Implemented a new API route for handling operator disbursement claims, including signature verification and interaction with the Polkadot node.
  • Added UI elements in WalletSideKick for users to claim operator disbursements, with state management for claim status and error handling.
  • Defined new constants for API routes and claim types.
  • Refactored Discord auth provider to use a new verifyToken utility function.
  • Added utility functions for handling claims and claim statistics in FaunaDB.
  • Introduced Slack utility functions to send notifications for claim stats and low wallet balance.
  • Updated .env.sample to include new environment variables for Slack integration and operator disbursement.

Changes walkthrough 📝

Relevant files
Enhancement
route.ts
Implement operator disbursement claim API route.                 

explorer/src/app/api/claim/[...params]/route.ts

  • Added a new API route for handling operator disbursement claims.
  • Implemented signature verification and Polkadot node interaction.
  • Added error handling and Slack notifications for claim status.
  • +105/-0 
    GetDiscordRoles.tsx
    Add UI for claiming operator disbursement in WalletSideKick.

    explorer/src/components/WalletSideKick/GetDiscordRoles.tsx

  • Added UI elements for claiming operator disbursement.
  • Integrated with the new claim API route.
  • Added state management for claim status and error handling.
  • +101/-9 
    routes.ts
    Define constants for new claim API routes.                             

    explorer/src/constants/routes.ts

    • Added new constants for API routes and claim types.
    +16/-0   
    discord.ts
    Refactor Discord auth provider to use verifyToken utility.

    explorer/src/utils/auth/providers/discord.ts

    • Refactored to use a new verifyToken utility function.
    +2/-14   
    verifyToken.ts
    Add utility function for JWT token verification.                 

    explorer/src/utils/auth/verifyToken.ts

    • Added a new utility function to verify JWT tokens.
    +20/-0   
    fauna.ts
    Add FaunaDB utilities for handling claims and statistics.

    explorer/src/utils/fauna.ts

  • Added functions to handle claims and claim statistics in FaunaDB.
  • Refactored existing functions to use generic types.
  • +102/-13
    slack.ts
    Add Slack utility functions for notifications.                     

    explorer/src/utils/slack.ts

  • Added utility functions to send Slack messages for claim stats and low
    wallet balance.
  • +99/-0   
    Documentation
    .env.sample
    Update .env.sample with Slack and disbursement variables.

    explorer/.env.sample

  • Added new environment variables for Slack integration and operator
    disbursement.
  • +6/-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 Jun 25, 2024 that may be closed by this pull request
    Copy link

    netlify bot commented Jun 25, 2024

    Deploy Preview for dev-astral ready!

    Name Link
    🔨 Latest commit 5c94e97
    🔍 Latest deploy log https://app.netlify.com/sites/dev-astral/deploys/668557bbeb7e3400084f1b07
    😎 Deploy Preview https://deploy-preview-668--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.

    @github-actions github-actions bot added documentation Improvements or additions to documentation enhancement New feature or request labels Jun 25, 2024
    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 4
    🧪 Relevant tests No
    🔒 Security concerns No
    ⚡ Key issues to review Possible Bug:
    The verifyToken function is used without passing any parameters, but it seems to expect a session token to verify. This could lead to runtime errors or incorrect behavior.
    Error Handling:
    The error handling in the POST method in route.ts could be improved by providing more specific error messages and logging for better debugging and user feedback.
    Security Concern:
    The use of environment variables directly in the code (e.g., process.env.WALLET_CLAIM_OPERATOR_DISBURSEMENT_URI) without validation or error handling could lead to potential security risks if the environment is not configured correctly.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Use a transaction to ensure atomic updates in updateClaimStats

    Consider using a transaction to ensure atomicity when updating claim stats, as multiple
    updates can lead to data inconsistencies.

    explorer/src/utils/fauna.ts [124-137]

     export const updateClaimStats = async (document: ExprArg, savedStats: SavedStats, user: User) => {
       const now = q.Now()
    -  return await updateData(document, {
    -    ...savedStats,
    -    totalClaims: savedStats.totalClaims + 1,
    -    users: [
    -      ...savedStats.users,
    +  return client.query(
    +    q.Let(
           {
    -        id: user.id,
    -        claimedAt: now,
    +        update: {
    +          totalClaims: q.Add(savedStats.totalClaims, 1),
    +          users: q.Append(savedStats.users, [{ id: user.id, claimedAt: now }]),
    +          updatedAt: now
    +        }
           },
    -    ],
    -    updatedAt: now,
    -  } as SavedStats)
    +      q.Update(document, { data: q.Var('update') })
    +    )
    +  ) as Promise<SavedStats>
     }
     
    Suggestion importance[1-10]: 10

    Why: Ensuring atomicity in updates is critical to prevent data inconsistencies, especially in concurrent environments. This suggestion significantly improves the reliability of the update operation.

    10
    Add specific error handling for blockchain API interactions

    To improve error handling, consider adding specific error messages related to the failure
    of the ApiPromise.create() and api.query.system.account() calls. This will help in
    debugging and understanding the specific point of failure when interacting with the
    blockchain API.

    explorer/src/app/api/claim/[...params]/route.ts [49]

    -const api = await ApiPromise.create({ provider: wsProvider })
    +let api;
    +try {
    +  api = await ApiPromise.create({ provider: wsProvider })
    +} catch (error) {
    +  console.error('Failed to create API instance:', error)
    +  throw new Error('Blockchain connection failed')
    +}
     
    Suggestion importance[1-10]: 9

    Why: Adding specific error handling for the blockchain API interactions significantly improves debugging and error tracing, which is crucial for maintaining a reliable system.

    9
    Error handling
    Enhance error handling in the queryIndex function

    Refactor the queryIndex function to handle errors more robustly by throwing exceptions or
    returning a structured error response instead of just logging to the console.

    explorer/src/utils/fauna.ts [53]

    -.catch((error: FaunaHttpErrorResponseContent) => console.log('error', error))
    +.catch((error: FaunaHttpErrorResponseContent) => {
    +  throw new Error(`Query failed: ${error.description}`);
    +})
     
    Suggestion importance[1-10]: 9

    Why: Improving error handling by throwing exceptions or returning structured error responses is crucial for robust application behavior and easier debugging.

    9
    Possible bug
    Add a check to ensure the session object is valid after retrieval

    Consider checking the validity of the session object immediately after it is retrieved by
    verifyToken(). This will ensure that the session is valid before proceeding with further
    operations, which depend on the session's validity.

    explorer/src/app/api/claim/[...params]/route.ts [22]

     const session = verifyToken()
    +if (!session) throw new Error('Session is invalid')
     
    Suggestion importance[1-10]: 8

    Why: This suggestion improves the robustness of the code by ensuring that the session object is valid before proceeding with further operations. This is crucial for preventing potential runtime errors related to invalid sessions.

    8
    Best practice
    Improve type definitions for better type safety and maintainability

    Consider using a more specific type than object for the claim and user fields in the
    SavedClaim type to ensure type safety and better code maintainability.

    explorer/src/utils/fauna.ts [15-20]

     type SavedClaim = {
       id: string
       chain: Chains
       claimType: string
    -  claim: object
    -  user: object
    -  tx: object
    +  claim: ClaimType // Define ClaimType appropriately
    +  user: UserType   // Define UserType appropriately
    +  tx: TxType       // Define TxType appropriately
       createdAt: Expr
       updatedAt: Expr
     }
     
    Suggestion importance[1-10]: 8

    Why: Using more specific types for claim and user fields enhances type safety and maintainability, which is important for catching errors early and improving code readability.

    8
    Use destructured assignment for environment variables to enhance code clarity

    Replace the direct usage of environment variables within the function with a destructured
    assignment at the beginning of the function. This makes the code cleaner and the
    dependencies of the function clearer.

    explorer/src/app/api/claim/[...params]/route.ts [53]

    -const wallet = keyring.addFromUri(process.env.WALLET_CLAIM_OPERATOR_DISBURSEMENT_URI)
    +const { WALLET_CLAIM_OPERATOR_DISBURSEMENT_URI, CLAIM_OPERATOR_DISBURSEMENT_AMOUNT } = process.env
    +const wallet = keyring.addFromUri(WALLET_CLAIM_OPERATOR_DISBURSEMENT_URI)
     
    Suggestion importance[1-10]: 7

    Why: This suggestion enhances code readability and maintainability by clearly defining the environment variables at the beginning of the function. However, it is not critical for functionality.

    7
    Improve type safety by specifying a more accurate type than any

    Replace the any type in the .then response handler with a more specific type to improve
    type safety.

    explorer/src/utils/fauna.ts [45-51]

    -.then((response: any) => {
    +.then((response: { data: ExprArg[] }) => {
       const resultsRefs = response.data
       if (resultsRefs.length === 0) return null
       const results = resultsRefs.map((ref: ExprArg) => q.Get(ref))
       return client.query(results) as Promise<{ ref: ExprArg; ts: number; data: T }[]>
     })
     
    Suggestion importance[1-10]: 7

    Why: Replacing any with a more specific type improves type safety, which helps in catching type-related errors during development, although it is a minor improvement.

    7
    Performance
    Refactor repeated string operations into a single split and slice operation

    Refactor the repeated splitting and slicing of pathname to extract chain and claimType
    into a single operation to enhance code efficiency and readability.

    explorer/src/app/api/claim/[...params]/route.ts [26-27]

    -const chain = pathname.split('/').slice(3)[0]
    -const claimType = pathname.split('/').slice(4)[0]
    +const pathComponents = pathname.split('/')
    +const chain = pathComponents[3]
    +const claimType = pathComponents[4]
     
    Suggestion importance[1-10]: 6

    Why: This refactoring improves code efficiency and readability by reducing redundant operations. While beneficial, it is a minor enhancement.

    6

    jfrank-summit
    jfrank-summit previously approved these changes Jun 25, 2024
    Copy link
    Member

    @jfrank-summit jfrank-summit left a comment

    Choose a reason for hiding this comment

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

    one small nit

    explorer/src/app/api/claim/[...params]/route.ts Outdated Show resolved Hide resolved
    jfrank-summit
    jfrank-summit previously approved these changes Jun 26, 2024
    @marc-aurele-besner marc-aurele-besner merged commit 7b1ea0b into main Jul 3, 2024
    13 checks passed
    @marc-aurele-besner marc-aurele-besner deleted the 633-add-flow-to-claim-operator-staking-token branch July 3, 2024 14:48
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    documentation Improvements or additions to documentation enhancement New feature or request Review effort [1-5]: 4
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    Add flow to claim operator staking token
    2 participants