Skip to content

[FIX] Load necessary atoms #11

Merged
fermuch merged 7 commits intomainfrom
fix/load-necessary-atoms
Feb 21, 2025
Merged

[FIX] Load necessary atoms #11
fermuch merged 7 commits intomainfrom
fix/load-necessary-atoms

Conversation

@Vinocis
Copy link
Copy Markdown
Collaborator

@Vinocis Vinocis commented Feb 20, 2025 •
edited by github-actions Bot
Loading

User description

String.to_existing_atom/1 was chosen instead of String.to_atom/1 because we do not have control over the strings being converted, given that they come from a external source (Numscript-WASM), which may lead into a overload on our atoms table. The problem is that we did not load some of the atoms needed in any part of the app, thus failing in some points.

The solution was create structs, which not only solves the problem, but improve the legibility as well.

PR Type

enhancement, bug_fix, tests

Description

  • Introduced Posting and Balance structs for better atom management.

  • Updated type specifications to use new structs.

  • Enhanced tests to validate new struct implementations.

  • Updated README to reflect new struct usage.

Changes walkthrough 📝

Relevant files
Enhancement
3 files
numscriptex.ex
Refactor to use `Posting` and `Balance` structs                   
+11/-25 
balance.ex
Implement `Balance` struct and related functions                 
+64/-39 
posting.ex
Implement `Posting` struct and conversion functions           
+38/-0   
Formatting
2 files
check_log.ex
Minor formatting changes                                                                 
+0/-1     
run.ex
Minor formatting changes                                                                 
+0/-1     
Tests
4 files
balance_test.exs
Update tests for `Balance` struct                                               
+25/-10 
check_log_test.exs
Correct test description for `CheckLog`                                   
+3/-2     
posting_tests.exs
Add tests for `Posting` struct                                                     
+58/-0   
numscriptex_test.exs
Update integration tests for new structs                                 
+227/-82
Documentation
1 files
README.md
Update documentation for new struct usage                               
+5/-5     
Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
    •

    @Vinocis Vinocis requested a review from fermuch February 20, 2025 20:11
    @Vinocis Vinocis self-assigned this Feb 20, 2025
    @github-actions
    Copy link
    Copy Markdown

    github-actions Bot commented Feb 20, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Possible Issue

    The calculate_final_balance function in Numscriptex.Balance might not handle cases where the asset or account is nil, which could lead to unexpected behavior or errors.

    defp calculate_final_balance(balance, posting, initial_balance) do
  same_asset? = posting["asset"] == balance[:asset]
  source? = posting["source"] == balance[:account]
  destination? = posting["destination"] == balance[:account]

  cond do
    source? and destination? and same_asset? ->
    Type Specification

    The from_list/1 function in Numscriptex.Posting has a duplicate @spec annotation, which could lead to confusion about the expected input and output types.

    @spec from_list(map()) :: list(__MODULE__.t())
def from_list(postings) do
  Enum.map(postings, &from_map/1)
end

@spec from_list(map()) :: __MODULE__.t()
def from_map(map) do

    @github-actions
    Copy link
    Copy Markdown

    github-actions Bot commented Feb 20, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Validate balance fields are non-negative

    Add validation to ensure that initial_balance and final_balance are non-negative
    integers to prevent incorrect balance calculations.

    lib/numscriptex/balance.ex [23-28]

     @type t() :: %__MODULE__{
         account: bitstring(),
         asset: bitstring(),
-        final_balance: pos_integer(),
-        initial_balance: pos_integer()
+        final_balance: non_neg_integer(),
+        initial_balance: non_neg_integer()
       }
    Suggestion importance[1-10]: 9

    __

    Why: The suggestion improves the type definition by ensuring that initial_balance and final_balance are non-negative integers, which is crucial for maintaining data integrity and preventing incorrect balance calculations.

    		High
    Handle empty postings list safely

    Ensure that the Balance.put/2 function handles cases where postings is an empty list
    to avoid potential runtime errors.

    lib/numscriptex.ex [136]

    -balances = Balance.put(initial_balance, postings)
+balances = if postings == [], do: [], else: Balance.put(initial_balance, postings)
    Suggestion importance[1-10]: 8

    __

    Why: This suggestion addresses a potential runtime error by ensuring that the Balance.put/2 function can handle cases where postings is an empty list. This is a significant improvement in terms of robustness and error handling.

    		Medium
    Validate required keys in map

    Ensure that the from_map/1 function checks for required keys in the input map to
    prevent incomplete Posting structs.

    lib/numscriptex/posting.ex [35-37]

     def from_map(map) do
+  required_keys = [:amount, :asset, :destination, :source]
+  Enum.each(required_keys, fn key ->
+    if Map.has_key?(map, key) == false, do: raise ArgumentError, "Missing key: #{key}"
+  end)
   struct(%__MODULE__{}, map)
 end
    Suggestion importance[1-10]: 8

    __

    Why: This suggestion enhances the from_map/1 function by adding a check for required keys, preventing the creation of incomplete Posting structs. This validation is important for ensuring data consistency and avoiding runtime errors due to missing keys.

    		Medium

    fermuch
    fermuch requested changes Feb 20, 2025
    View reviewed changes
    Comment thread lib/numscriptex/balance.ex Outdated
    Comment thread lib/numscriptex/posting.ex
    Comment thread lib/numscriptex/posting.ex
    Comment thread test/numscriptex/check_log_test.exs Outdated
    Comment thread lib/numscriptex/balance.ex Outdated
    Comment thread lib/numscriptex/balance.ex Outdated
    Comment thread test/numscriptex/posting_tests.exs Outdated
    @Vinocis Vinocis requested a review from fermuch February 20, 2025 20:37
    fermuch
    fermuch requested changes Feb 20, 2025
    View reviewed changes
    Comment thread lib/numscriptex/balance.ex Outdated
    @Vinocis Vinocis requested a review from fermuch February 20, 2025 22:24
    fermuch
    fermuch approved these changes Feb 21, 2025
    View reviewed changes
    Copy link
    Copy Markdown
    Contributor

    @fermuch fermuch left a comment

    Choose a reason for hiding this comment

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

    LGTM!

    @fermuch fermuch merged commit 2e0f726 into main Feb 21, 2025
    @fermuch fermuch deleted the fix/load-necessary-atoms branch February 21, 2025 12:19
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @fermuch fermuch fermuch approved these changes

    Assignees

    @Vinocis Vinocis

    Labels

    Review effort 3/5

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    2 participants

    @Vinocis @fermuch