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

[PLA-1840] add account update for sign #131

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

zlayine
Copy link
Contributor

@zlayine zlayine commented Jun 28, 2024

PR Type

Enhancement, Bug fix


Description

  • Updated SignTransaction.vue to include logic for updating user wallet account if the application is multi-tenant.
  • Added updateUserAccount method to AuthApi class in auth.ts.
  • Modified UpdateUser GraphQL mutation to accept an account variable.
  • Added updateUserAccount method to useAppStore in index.ts.

Changes walkthrough 📝

Relevant files
Enhancement
SignTransaction.vue
Update user wallet account in multi-tenant setup                 

resources/js/components/SignTransaction.vue

  • Imported publicKeyToAddress from ~/util/address.
  • Added logic to update user wallet account if multi-tenant.
  • +6/-1     
    auth.ts
    Add updateUserAccount method in AuthApi                                   

    resources/js/api/auth.ts

    • Added updateUserAccount method to AuthApi class.
    +11/-0   
    UpdateUser.ts
    Extend UpdateUser mutation to include account variable     

    resources/js/graphql/mutation/auth/UpdateUser.ts

    • Modified UpdateUser mutation to include account variable.
    +2/-2     
    index.ts
    Add updateUserAccount method in useAppStore                           

    resources/js/store/index.ts

    • Added updateUserAccount method to useAppStore.
    +3/-0     

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

    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 3
    🧪 Relevant tests No
    🔒 Security concerns No
    ⚡ Key issues to review Possible Bug:
    The updateUserAccount method in AuthApi does not handle errors. It would be beneficial to include error handling to manage API request failures gracefully.
    Data Validation:
    The updateUserAccount method does not validate the address parameter before sending it in the GraphQL mutation. Ensuring the address format could prevent potential issues.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add error handling for the updateUserAccount method

    Consider adding error handling for the updateUserAccount method to manage potential
    failures gracefully.

    resources/js/components/SignTransaction.vue [122]

    -await useAppStore().updateUserAccount(publicKeyToAddress(account.address));
    +try {
    +    await useAppStore().updateUserAccount(publicKeyToAddress(account.address));
    +} catch (error) {
    +    console.error('Failed to update user account:', error);
    +    // Handle error appropriately
    +}
     
    Suggestion importance[1-10]: 9

    Why: Adding error handling for the updateUserAccount method is crucial for managing potential failures gracefully, improving the robustness of the application.

    9
    Enhancement
    Handle responses in updateUserAccount to ensure successful updates

    Ensure that the updateUserAccount method handles the response from sendPlatfromRequest to
    verify successful updates or handle errors.

    resources/js/api/auth.ts [167]

    -return AuthApi.sendPlatfromRequest(data);
    +const response = await AuthApi.sendPlatfromRequest(data);
    +if (!response.success) {
    +    throw new Error('Failed to update user account');
    +}
    +return response;
     
    Suggestion importance[1-10]: 8

    Why: Ensuring that the updateUserAccount method handles the response from sendPlatfromRequest is important for verifying successful updates and handling errors, enhancing the reliability of the method.

    8
    Best practice
    Enhance the GraphQL mutation to include error handling and response fields

    Add error handling for the GraphQL mutation to manage potential failures and ensure data
    integrity.

    resources/js/graphql/mutation/auth/UpdateUser.ts [2]

    -UpdateUser(email: $email, password: $password, account: $account)
    +UpdateUser(email: $email, password: $password, account: $account) {
    +    id
    +    email
    +    account
    +}
     
    Suggestion importance[1-10]: 7

    Why: Adding response fields to the GraphQL mutation can help in verifying the success of the mutation and managing potential failures, although the suggestion does not directly address error handling.

    7
    Maintainability
    Add a return statement to updateUserAccount for better control flow

    Consider adding a return statement for the updateUserAccount method to enable chaining and
    better control flow handling.

    resources/js/store/index.ts [254]

    -await AuthApi.updateUserAccount(address);
    +return await AuthApi.updateUserAccount(address);
     
    Suggestion importance[1-10]: 6

    Why: Adding a return statement to the updateUserAccount method can improve control flow and enable chaining, enhancing the maintainability of the code.

    6

    Copy link
    Contributor

    @enjinabner enjinabner left a comment

    Choose a reason for hiding this comment

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

    updates

    export default `mutation UpdateUser($email: String, $password: String) {
    UpdateUser(email: $email, password: $password)
    export default `mutation UpdateUser($email: String, $password: String, $account: String) {
    UpdateUser(email: $email, password: $password, account: $account)
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    This attribute is dedicated for wallet daemon and we cannot update this. I created a new attribute for this in this PR https://github.com/enjin/platform-multi-tenant/pull/72

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    okay thank you! will check the new implementation

    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.

    None yet

    2 participants