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-2008] api 2.0 #161

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

[PLA-2008] api 2.0 #161

wants to merge 8 commits into from

Conversation

zlayine
Copy link
Contributor

@zlayine zlayine commented Sep 23, 2024

PR Type

enhancement, bug fix


Description

  • Enhanced wallet connection logic to support multi-tenant environments.
  • Simplified transfer balance logic by removing the keepAlive option.
  • Removed display of unitPrice and minimumBalance in token details.
  • Added pagination support to collection IDs fetching.
  • Renamed and simplified the transfer balance mutation.
  • Added a method to fetch unique trackable accounts.
  • Enhanced collection fetching with pagination and account filtering.

Changes walkthrough 📝

Relevant files
Enhancement
WalletConnectButton.vue
Enhance wallet connection with multi-tenant support           

resources/js/components/WalletConnectButton.vue

  • Added a check for multi-tenant before setting user accounts.
  • Initialized walletAccounts to an empty array if undefined.
  • +4/-3     
    TransferBalanceSlideover.vue
    Simplify transfer balance logic by removing keepAlive       

    resources/js/components/slideovers/common/TransferBalanceSlideover.vue

  • Removed keepAlive checkbox and related logic.
  • Simplified transfer balance mutation call.
  • +0/-9     
    DetailsTokenSlideover.vue
    Remove unit price and minimum balance display                       

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

  • Removed display of unitPrice and minimumBalance.
  • Cleaned up unused imports and variables.
  • +0/-23   
    collection.ts
    Add pagination support to collection IDs fetch                     

    resources/js/api/collection.ts

    • Added after parameter to getCollectionsIds method.
    +2/-1     
    TransferBalance.ts
    Rename and simplify transfer balance mutation                       

    resources/js/graphql/mutation/TransferBalance.ts

  • Renamed mutation from TransferBalance to TransferAllowDeath.
  • Removed keepAlive parameter.
  • +2/-3     
    GetTokens.ts
    Simplify token query by removing unused fields                     

    resources/js/graphql/query/GetTokens.ts

    • Removed minimumBalance and unitPrice fields from token query.
    +0/-2     
    connection.ts
    Add method to fetch trackable accounts                                     

    resources/js/store/connection.ts

    • Added getTrackableAccounts method to fetch unique accounts.
    +21/-0   
    index.ts
    Enhance collection fetching with pagination and account filtering

    resources/js/store/index.ts

  • Enhanced fetchCollectionIds to support pagination.
  • Integrated trackable accounts filtering in collection fetching.
  • +18/-6   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    PR Reviewer Guide 🔍

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

    Possible Bug
    The new multi-tenant check and setting user accounts might not handle errors or exceptions, potentially leading to unhandled promise rejections.

    Code Removal
    The removal of the keepAlive option changes the behavior of balance transfers. Ensure this change is documented and tested, especially if it affects existing functionalities.

    API Change
    The addition of pagination support in getCollectionsIds method introduces new variables and logic. Ensure that the pagination logic is correctly implemented and tested.

    Mutation Rename
    The mutation has been renamed from TransferBalance to TransferAllowDeath. This is a significant change and should be thoroughly tested to ensure it does not break existing functionalities.

    New Method
    The new method getTrackableAccounts adds significant logic for account management. It should be reviewed for performance implications, especially with large data sets.

    Copy link

    github-actions bot commented Sep 23, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add a check for useAppStore().user to prevent accessing properties on undefined

    Consider checking for the existence of useAppStore().user before accessing
    walletAccounts to avoid potential runtime errors if user is undefined.

    resources/js/components/WalletConnectButton.vue [115]

    -const walletAccounts = useAppStore().user?.walletAccounts?.map((account) => publicKeyToAddress(account)) ?? [];
    +const walletAccounts = useAppStore().user ? useAppStore().user.walletAccounts?.map((account) => publicKeyToAddress(account)) ?? [] : [];
     
    Suggestion importance[1-10]: 8

    Why: This suggestion addresses a potential runtime error by ensuring that useAppStore().user is defined before accessing its properties, which is a good practice for preventing bugs.

    8
    Safeguard the creation of uniqueAccounts against potential undefined values

    Ensure that uniqueAccounts is properly updated within the try block to handle cases
    where walletAccounts or localAccounts might be empty or undefined.

    resources/js/components/WalletConnectButton.vue [116]

    -const uniqueAccounts = [...new Set([...walletAccounts, ...localAccounts])];
    +const uniqueAccounts = [...new Set([...(walletAccounts || []), ...(localAccounts || [])])];
     
    Suggestion importance[1-10]: 6

    Why: This suggestion provides a safeguard against potential undefined values, which can prevent errors, although the existing code already handles empty arrays.

    6
    Performance
    Store the result of useAppStore() in a variable to improve performance and code readability

    Instead of calling useAppStore() multiple times, store its result in a variable at
    the start of the function to optimize performance and readability.

    resources/js/components/WalletConnectButton.vue [115-119]

    -const walletAccounts = useAppStore().user?.walletAccounts?.map((account) => publicKeyToAddress(account)) ?? [];
    -if (useAppStore().isMultiTenant) {
    +const appStore = useAppStore();
    +const walletAccounts = appStore.user?.walletAccounts?.map((account) => publicKeyToAddress(account)) ?? [];
    +if (appStore.isMultiTenant) {
         AuthApi.setUserAccounts(uniqueAccounts);
     }
     
    Suggestion importance[1-10]: 7

    Why: This suggestion improves performance and readability by reducing redundant calls to useAppStore(), which is a beneficial optimization.

    7
    Enhancement
    Use Set for both walletAccounts and localAccounts to ensure uniqueness before merging

    To ensure that uniqueAccounts only contains unique values, consider using a Set for
    walletAccounts and localAccounts before merging them.

    resources/js/components/WalletConnectButton.vue [116]

    -const uniqueAccounts = [...new Set([...walletAccounts, ...localAccounts])];
    +const walletAccountsSet = new Set(walletAccounts);
    +const localAccountsSet = new Set(localAccounts);
    +const uniqueAccounts = [...new Set([...walletAccountsSet, ...localAccountsSet])];
     
    Suggestion importance[1-10]: 5

    Why: While using a Set for both walletAccounts and localAccounts before merging is a valid approach, the existing code already ensures uniqueness, making this suggestion a minor enhancement.

    5

    enjinabner
    enjinabner previously approved these changes Sep 25, 2024
    enjinabner
    enjinabner previously approved these changes Sep 27, 2024
    enjinabner
    enjinabner previously approved these changes Oct 4, 2024
    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