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-2125] Fix probablities format #120

Merged
merged 2 commits into from
Jan 7, 2025

Conversation

enjinabner
Copy link
Contributor

@enjinabner enjinabner commented Jan 6, 2025

PR Type

Enhancement, Bug fix


Description

  • Added formatFtTokenIds method to handle token ID ranges.

  • Updated probabilities attribute to use formatted token IDs.

  • Fixed minor formatting issues in comments and annotations.

  • Introduced HasIntegerRanges trait to Beam model.


Changes walkthrough 📝

Relevant files
Formatting
BeamClaimPending.php
Fix comment formatting in BeamClaimPending                             

src/Events/BeamClaimPending.php

  • Fixed a minor formatting issue in a comment.
+1/-1     
BeamExists.php
Fix annotation formatting in BeamExists rule                         

src/Rules/BeamExists.php

  • Fixed formatting in a method annotation.
+1/-1     
Enhancement
Beam.php
Enhance token ID handling in Beam model                                   

src/Models/Laravel/Beam.php

  • Added formatFtTokenIds method for token ID range handling.
  • Updated probabilities attribute to use formatted token IDs.
  • Introduced HasIntegerRanges trait to the model.
  • +23/-1   

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

    Copy link

    github-actions bot commented Jan 6, 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

    Possible Issue

    The formatFtTokenIds method introduced in this PR may not handle edge cases where the expandRanges method fails or returns unexpected results. Ensure this behavior is tested and validated.

    protected function formatFtTokenIds(array $value): array
    {
        if (empty($value)) {
            return $value;
        }
    
        $formatted = [];
        foreach ($value as $key => $val) {
            if (str_contains($key, '..')) {
                foreach ($this->expandRanges($key) as $tokenId) {
                    $formatted[$tokenId] = $val;
                }
            } else {
                $formatted[$key] = $val;
            }
        }
    
        return $formatted;
    }
    Data Consistency

    The addition of ft in the probabilities attribute relies on the formatFtTokenIds method. Ensure this change does not introduce inconsistencies with existing data or break compatibility with other parts of the system.

    protected function probabilities(): Attribute
    {
        $probabilities = ClaimProbabilities::getProbabilities($this->code)['probabilities'] ?? null;
    
        return Attribute::make(
            get: fn () => $probabilities ? [
                'ft' => (object) $this->formatFtTokenIds((array) $probabilities['ft']),
                'nft' => $probabilities['nft'],

    Copy link

    github-actions bot commented Jan 6, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Safeguard against null responses from ClaimProbabilities::getProbabilities to prevent undefined key access

    Handle cases where ClaimProbabilities::getProbabilities($this->code) returns null to
    avoid accessing undefined array keys.

    src/Models/Laravel/Beam.php [152]

    -$probabilities = ClaimProbabilities::getProbabilities($this->code)['probabilities'] ?? null;
    +$probabilitiesData = ClaimProbabilities::getProbabilities($this->code);
    +$probabilities = $probabilitiesData['probabilities'] ?? null;
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential bug where accessing undefined keys could lead to runtime errors. It is a critical improvement for ensuring the code handles null responses gracefully.

    9
    Verify the existence and proper implementation of the expandRanges method to prevent runtime issues

    Ensure that the expandRanges method used in the formatFtTokenIds function is
    implemented and correctly handles the range expansion logic to avoid runtime errors.

    src/Models/Laravel/Beam.php [173-175]

    -foreach ($this->expandRanges($key) as $tokenId) {
    -    $formatted[$tokenId] = $val;
    +if (method_exists($this, 'expandRanges')) {
    +    foreach ($this->expandRanges($key) as $tokenId) {
    +        $formatted[$tokenId] = $val;
    +    }
    +} else {
    +    throw new \RuntimeException('expandRanges method is not implemented.');
     }
    Suggestion importance[1-10]: 8

    Why: The suggestion ensures that the expandRanges method is implemented and avoids potential runtime errors if the method is missing. This is a critical safeguard for the functionality of the formatFtTokenIds method.

    8
    General
    Add validation for the ft probabilities array to ensure it is properly structured before processing

    Validate the structure of the $probabilities['ft'] array before passing it to
    formatFtTokenIds to avoid unexpected errors during processing.

    src/Models/Laravel/Beam.php [156]

    -'ft' => (object) $this->formatFtTokenIds((array) $probabilities['ft']),
    +'ft' => isset($probabilities['ft']) && is_array($probabilities['ft']) 
    +    ? (object) $this->formatFtTokenIds($probabilities['ft']) 
    +    : null,
    Suggestion importance[1-10]: 7

    Why: Validating the structure of the ft probabilities array prevents unexpected errors during processing, improving the robustness of the code. This is a useful enhancement for handling edge cases.

    7

    src/Models/Laravel/Beam.php Outdated Show resolved Hide resolved
    @enjinabner enjinabner merged commit c822fa2 into master Jan 7, 2025
    5 of 7 checks passed
    @enjinabner enjinabner deleted the feature/pla-2125/fix-probabilities-format branch January 7, 2025 12:56
    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