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-2019] Adds token struct to TokenType #259

Merged
merged 5 commits into from
Oct 4, 2024
Merged

Conversation

leonardocustodio
Copy link
Member

@leonardocustodio leonardocustodio commented Sep 29, 2024

PR Type

enhancement, tests


Description

  • Enhanced the tokens table with new fields and constraints, including requires_deposit and creation_depositor.
  • Introduced TokenMetadataType in the GraphQL schema to handle token metadata.
  • Extended TokenType with new fields for deposits and metadata.
  • Updated various tests to reflect the removal of deprecated fields and the addition of new fields.
  • Revised TokenStorage.json to align with the updated token data structure.

Changes walkthrough 📝

Relevant files
Enhancement
8 files
2024_09_17_164605_upgrade_tokens_table.php
Enhance tokens table with new fields and constraints         

database/migrations/2024_09_17_164605_upgrade_tokens_table.php

  • Added new fields to the tokens table.
  • Introduced foreign key constraint for creation_depositor.
  • Dropped unused columns unit_price and minimum_balance.
  • +12/-5   
    TokenMetadataType.php
    Introduce TokenMetadataType for GraphQL schema                     

    src/GraphQL/Types/Substrate/TokenMetadataType.php

  • Added new TokenMetadataType class.
  • Defined fields for token metadata including name, symbol, and
    decimalCount.
  • +46/-0   
    TokenType.php
    Extend TokenType with deposit and metadata fields               

    src/GraphQL/Types/Substrate/TokenType.php

  • Added fields related to deposits and metadata.
  • Implemented new fields for requiresDeposit, creationDeposit, and
    tokenMetadata.
  • +46/-0   
    EagerLoadSelectFields.php
    Update EagerLoadSelectFields for new token attributes       

    src/Models/Laravel/Traits/EagerLoadSelectFields.php

  • Updated loadToken method to include new fields.
  • Added support for selecting creationDepositor and tokenMetadata.
  • +6/-0     
    Token.php
    Add creationDepositor relationship to Token model               

    src/Models/Laravel/Traits/Token.php

    • Added creationDepositor relationship method.
    +8/-0     
    Decoder.php
    Update Decoder for new token storage data structure           

    src/Services/Processor/Substrate/Codec/Decoder.php

  • Updated tokenStorageData to process new fields.
  • Removed deprecated fields and added new metadata fields.
  • +12/-13 
    Parser.php
    Enhance Parser to handle new token fields                               

    src/Services/Processor/Substrate/Parser.php

  • Added handling for creationDepositor in token storage parsing.
  • Updated token data insertion with new fields.
  • +15/-4   
    TokenStorage.json
    Revise TokenStorage.json for updated token data structure

    src/Services/Processor/Substrate/Codec/Types/TokenStorage.json

  • Updated TokenStorageData structure.
  • Removed deprecated fields and added new metadata fields.
  • +9/-39   
    Tests
    7 files
    GetTokenTest.php
    Update GetTokenTest to reflect schema changes                       

    tests/Feature/GraphQL/Queries/GetTokenTest.php

    • Removed assertions for deprecated fields.
    +1/-4     
    GetTokensTest.php
    Update GetTokensTest to reflect schema changes                     

    tests/Feature/GraphQL/Queries/GetTokensTest.php

    • Removed assertions for deprecated fields.
    +1/-4     
    GetWalletTest.php
    Update GetWalletTest to reflect schema changes                     

    tests/Feature/GraphQL/Queries/GetWalletTest.php

    • Removed assertions for deprecated fields.
    +0/-2     
    EncodingTest.php
    Update EncodingTest with new sequence mode                             

    tests/Unit/EncodingTest.php

    • Added mode to the fake signature sequence.
    +2/-1     
    GetToken.graphql
    Update GetToken GraphQL query with new fields                       

    tests/Feature/GraphQL/Resources/GetToken.graphql

    • Added new fields related to deposits and metadata.
    +19/-3   
    GetTokens.graphql
    Update GetTokens GraphQL query with new fields                     

    tests/Feature/GraphQL/Resources/GetTokens.graphql

    • Added new fields related to deposits and metadata.
    +19/-3   
    GetWallet.graphql
    Update GetWallet GraphQL query with new fields                     

    tests/Feature/GraphQL/Resources/GetWallet.graphql

    • Added new fields related to deposits and metadata.
    +19/-3   

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

    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Database Constraint
    The new field creation_depositor is constrained to the wallets table with cascading updates and deletes. Ensure that this behavior is intended and that it won't lead to unintended deletions or modifications in the wallets table.

    GraphQL Type Change
    The GraphQL type for depositor in CreationDepositType has been changed from non-nullable (Wallet!) to nullable (Wallet). This change could affect the client applications expecting a non-null value always.

    Data Processing Change
    The method tokenStorageData now processes data using a new codec TokenStorageData instead of the previous TokenStorageDataV1010. Verify that all necessary data fields are correctly handled and that no data is lost or misinterpreted during the transition.

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Enhance data integrity by handling potential null values in wallet data

    Add error handling for cases where depositorWallet might be null, especially if the
    creationDepositor does not exist or fails to fetch, to avoid inserting invalid or
    incomplete data.

    src/Services/Processor/Substrate/Parser.php [187]

    -'creation_depositor' => $depositorWallet?->id,
    +// Handle potential null value for depositorWallet
    +'creation_depositor' => $depositorWallet?->id ?? throw new Exception("Depositor wallet not found"),
    Suggestion importance[1-10]: 8

    Why: Adding error handling for potential null values in depositorWallet is crucial for maintaining data integrity and preventing the insertion of invalid data. This suggestion addresses a potential bug and improves the reliability of the code.

    8
    Prevent errors by checking for key existence before data access

    Validate the existence of keys like 'creationDeposit.depositor' in the decoded data
    before accessing them to prevent potential errors from missing data.

    src/Services/Processor/Substrate/Codec/Decoder.php [248]

    -'creationDepositor' => ($depositor = Arr::get($decoded, 'creationDeposit.depositor')) !== null ? HexConverter::prefix($depositor) : null,
    +// Check if 'creationDeposit.depositor' exists before trying to access it
    +'creationDepositor' => Arr::has($decoded, 'creationDeposit.depositor') ? HexConverter::prefix(Arr::get($decoded, 'creationDeposit.depositor')) : null,
    Suggestion importance[1-10]: 7

    Why: The suggestion to check for key existence before accessing data is a good practice to prevent potential errors from missing data. It enhances the robustness of the code by ensuring that operations are only performed on existing data.

    7
    Enhancement
    Reinforce data integrity by ensuring the depositor field cannot be null

    Consider reverting the GraphQL type of depositor to 'Wallet!' to ensure
    non-nullability, which aligns with the expected data integrity and avoids potential
    null reference errors in client applications.

    src/GraphQL/Types/Substrate/CreationDepositType.php [33-34]

     'depositor' => [
    -    'type' => GraphQL::type('Wallet'),
    +    'type' => GraphQL::type('Wallet!'),
         'description' => __('enjin-platform::type.creation_deposit.description'),
     ],
    Suggestion importance[1-10]: 5

    Why: Changing the GraphQL type of depositor to non-nullable can help ensure data integrity and prevent null reference errors. However, this change might not be suitable if null values are expected in some scenarios, so it requires careful consideration.

    5
    Possible issue
    Ensure the foreign key setup is referencing an existing column

    Ensure that the foreign key creation_depositor references an existing column in the
    wallets table to prevent runtime errors due to missing relations.

    database/migrations/2024_09_17_164605_upgrade_tokens_table.php [16-22]

    +// Ensure the 'wallets' table and 'id' column exist and are correctly set up before applying this constraint
     $table->foreignId('creation_depositor')
         ->nullable()
         ->index()
         ->after('requires_deposit')
         ->constrained('wallets')
         ->cascadeOnUpdate()
         ->cascadeOnDelete();
    Suggestion importance[1-10]: 3

    Why: The suggestion to ensure the foreign key references an existing column is a good practice for preventing runtime errors, but it doesn't introduce any new checks or changes to the code. It merely adds a comment, which has a minimal impact on the code's functionality.

    3

    💡 Need additional feedback ? start a PR chat

    enjinabner
    enjinabner previously approved these changes Sep 30, 2024
    Base automatically changed from PLA-2018 to master October 1, 2024 11:45
    @leonardocustodio leonardocustodio dismissed enjinabner’s stale review October 1, 2024 11:45

    The base branch was changed.

    @leonardocustodio leonardocustodio merged commit bca3c22 into master Oct 4, 2024
    7 checks passed
    @leonardocustodio leonardocustodio deleted the PLA-2019 branch October 4, 2024 13:28
    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.

    3 participants