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-1837] Add account owner validation #194

Merged
merged 2 commits into from
Jul 3, 2024

Conversation

enjinabner
Copy link
Contributor

@enjinabner enjinabner commented Jul 3, 2024

PR Type

Enhancement, Bug fix


Description

  • Updated the collection owner validation logic by replacing SS58Address::isSameAddress with Account::isAccountOwner.
  • Added a new static property $walletAccounts in the Account class.
  • Introduced a new method isAccountOwner in the Account class to check if an account is the owner.

Changes walkthrough 📝

Relevant files
Enhancement
IsCollectionOwner.php
Update collection owner validation logic                                 

src/Rules/IsCollectionOwner.php

  • Replaced SS58Address::isSameAddress with Account::isAccountOwner for
    owner validation.
  • Removed the import of SS58Address.
  • +1/-2     
    IsCollectionOwnerOrApproved.php
    Update collection owner or approval validation logic         

    src/Rules/IsCollectionOwnerOrApproved.php

  • Replaced SS58Address::isSameAddress with Account::isAccountOwner for
    owner or approval validation.
  • Removed the import of SS58Address.
  • +1/-2     
    Account.php
    Add wallet accounts and account owner validation method   

    src/Support/Account.php

  • Added a new static property $walletAccounts.
  • Added a new method isAccountOwner to check if an account is the owner.

  • +20/-0   

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

    Copy link

    github-actions bot commented Jul 3, 2024

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 2
    🧪 Relevant tests No
    🔒 Security concerns No
    ⚡ Key issues to review Possible Bug:
    The isAccountOwner method in Account.php merges arrays $publicKey, $walletAccounts, and $others incorrectly. $publicKey is a single string, not an array, which could lead to errors when attempting to merge. This should be wrapped in an array or handled differently.
    Code Clarity:
    The isAccountOwner method uses a variable $others which is a string. The method signature and the comment should clarify if $others can be multiple values and how they are expected to be passed to the function.

    Copy link

    github-actions bot commented Jul 3, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Correct the array merging to prevent runtime errors

    Modify the isAccountOwner method to correctly merge arrays. Currently, the method attempts
    to merge a string ($others) with arrays, which will cause an error. Instead, ensure that
    all elements being merged are arrays.

    src/Support/Account.php [21-24]

     $accounts = array_merge(
    -    static::$publicKey,
    +    (array)static::$publicKey,
         static::$walletAccounts,
    -    $others
    +    (array)$others
     );
     
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential runtime error by ensuring that all elements being merged are arrays, which is crucial for the correct functioning of the isAccountOwner method.

    9
    Add a null check for the owner's public key to prevent exceptions

    Add a null check for $collection->owner before accessing its public_key property to avoid
    potential null reference exceptions.

    src/Rules/IsCollectionOwner.php [33-35]

    -(!$collection->owner || !Account::isAccountOwner(
    +(!$collection->owner || !$collection->owner->public_key || !Account::isAccountOwner(
         $collection->owner->public_key,
         Arr::get($this->data, 'signingAccount') ?: Account::daemonPublicKey()
     ))
     
    Suggestion importance[1-10]: 8

    Why: Adding a null check for $collection->owner->public_key is important to prevent potential null reference exceptions, enhancing the robustness of the code.

    8
    Enhancement
    Update the method used for address comparison to a more current version

    Replace the deprecated SS58Address::isSameAddress method with a more current and supported
    method if available, or update its implementation to ensure compatibility.

    src/Support/Account.php [27]

    -if ($account && SS58Address::isSameAddress($publicKey, $account)) {
    +if ($account && NewAddressMethod::isSameAddress($publicKey, $account)) {
     
    Suggestion importance[1-10]: 6

    Why: While updating deprecated methods is generally good practice, the suggestion lacks specificity about the new method to use, which reduces its immediate applicability.

    6

    @enjinabner enjinabner marked this pull request as draft July 3, 2024 05:54
    @enjinabner enjinabner marked this pull request as ready for review July 3, 2024 06:19
    @enjinabner enjinabner merged commit 638ed75 into master Jul 3, 2024
    5 checks passed
    @enjinabner enjinabner deleted the feature/pla-1837/add-wallet-accounts branch July 3, 2024 13:40
    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