Skip to content

Conversation

@zlayine
Copy link
Contributor

@zlayine zlayine commented Feb 3, 2025

PR Type

Enhancement, Bug fix


Description

  • Added signingAccount field to the Burn Token functionality.

  • Updated validation schema to include signingAccount.

  • Modified GraphQL mutation to support signingAccount parameter.

  • Adjusted API and UI components to handle signingAccount.


Changes walkthrough 📝

Relevant files
Enhancement
BurnTokenSlideover.vue
Add `signingAccount` input and validation in UI                   

resources/js/components/slideovers/token/BurnTokenSlideover.vue

  • Added signingAccount input field to the UI.
  • Updated validation schema to include signingAccount.
  • Passed signingAccount to the burn token function.
  • +10/-0   
    token.ts
    Add `signingAccount` to API parameters                                     

    resources/js/api/token.ts

    • Included signingAccount in the API call parameters.
    +1/-0     
    BurnToken.ts
    Add `signingAccount` to GraphQL mutation                                 

    resources/js/graphql/mutation/token/BurnToken.ts

    • Updated GraphQL mutation to include signingAccount parameter.
    +2/-1     

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @github-actions
    Copy link

    github-actions bot commented Feb 3, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Validation Schema Update

    Ensure that the signingAccount field added to the validation schema is correctly handled and validated, especially since it is marked as not required.

    const validation = yup.object({
        tokenId: stringRequiredSchema,
        collectionId: collectionIdRequiredSchema,
        amount: numberRequiredSchema.min(1).typeError('Amount must be a number'),
        keepAlive: booleanNotRequiredSchema,
        removeTokenStorage: booleanNotRequiredSchema,
        signingAccount: addressNotRequiredSchema,
        idempotencyKey: stringNotRequiredSchema,
        skipValidation: booleanNotRequiredSchema,
    });
    GraphQL Mutation Update

    Verify that the addition of the signingAccount parameter in the GraphQL mutation is correctly implemented and does not break existing functionality.

    export default `mutation Burn($collectionId: BigInt!, $params: BurnParamsInput!, $signingAccount: String, $idempotencyKey: String, $skipValidation: Boolean! = false) {
        Burn(
          collectionId: $collectionId
          params: $params
          signingAccount: $signingAccount
          idempotencyKey: $idempotencyKey
          skipValidation: $skipValidation
        ) {
          id

    @github-actions
    Copy link

    github-actions bot commented Feb 3, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add validation for wallet address format

    Ensure that the signingAccount input field is validated for proper format (e.g.,
    valid wallet address) before being submitted to avoid potential errors or misuse.

    resources/js/components/slideovers/token/BurnTokenSlideover.vue [52-57]

     <FormInput
         v-model="signingAccount"
         name="signingAccount"
         label="Signing account"
         description="The wallet used to sign and broadcast the transaction. By default, this is the wallet daemon."
    +    :rules="[validateWalletAddress]"
     />
    Suggestion importance[1-10]: 8

    Why: Adding validation for the signingAccount input field ensures that only valid wallet addresses are submitted, reducing the risk of errors or misuse. This is a significant improvement in terms of data integrity and user input validation.

    8
    General
    Ensure signingAccount is optional

    Verify that the signingAccount parameter is optional and does not cause issues when
    omitted, as it is newly introduced in the mutation.

    resources/js/graphql/mutation/token/BurnToken.ts [1-9]

    -export default `mutation Burn($collectionId: BigInt!, $params: BurnParamsInput!, $signingAccount: String, $idempotencyKey: String, $skipValidation: Boolean! = false) {
    +export default `mutation Burn($collectionId: BigInt!, $params: BurnParamsInput!, $signingAccount: String = null, $idempotencyKey: String, $skipValidation: Boolean! = false) {
         Burn(
           collectionId: $collectionId
           params: $params
           signingAccount: $signingAccount
           idempotencyKey: $idempotencyKey
           skipValidation: $skipValidation
         ) {
           id
    Suggestion importance[1-10]: 7

    Why: Making the signingAccount parameter optional by providing a default value ensures backward compatibility and prevents potential issues when the parameter is omitted. This is a valuable enhancement for robustness.

    7
    Handle signingAccount parameter safely

    Confirm that the signingAccount parameter is correctly passed and handled in the API
    request to prevent potential runtime errors or misconfigurations.

    resources/js/api/token.ts [65]

    -signingAccount: tokenData.signingAccount,
    +signingAccount: tokenData.signingAccount || null,
    Suggestion importance[1-10]: 6

    Why: Safely handling the signingAccount parameter by providing a fallback value prevents potential runtime errors when the parameter is undefined. While this is a minor improvement, it enhances the reliability of the API request handling.

    6

    @zlayine zlayine merged commit d50774e into master Feb 3, 2025
    4 checks passed
    @zlayine zlayine deleted the bugfix/PLA-1840/burn-token-fix branch February 3, 2025 13:29
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Development

    Successfully merging this pull request may close these issues.

    2 participants