Skip to content

Conversation

@Vinocis
Copy link
Collaborator

@Vinocis Vinocis commented Apr 15, 2025

User description

We were lacking some validations on the Numscriptex.Builder which was leading to some unhandled errors (e.g. accepting empty strings on assets and account fields, etc).

This PR solves this by creating new validations and increasing the test coverage


PR Type

Bug fix, Tests, Documentation


Description

  • Added validations for metadata fields in Numscriptex.Builder.

  • Enhanced error handling for invalid inputs.

  • Increased test coverage for various edge cases.

  • Updated documentation links in code and README.


Changes walkthrough 📝

Relevant files
Bug fix
builder.ex
Add validations and improve error handling in Builder       

lib/numscriptex/builder.ex

  • Added validations for metadata fields.
  • Improved error handling for script building.
  • Refactored functions to return error tuples.
  • Enhanced logic for handling nested splits.
  • +137/-55
    Tests
    builder_test.exs
    Add comprehensive tests for Builder validations                   

    test/numscriptex/builder_test.exs

  • Added tests for new validation cases.
  • Increased coverage for error scenarios.
  • Tested handling of nested splits.
  • Verified error messages for invalid inputs.
  • +334/-0 
    Documentation
    README.md
    Update documentation links in README                                         

    README.md

  • Updated documentation link for Builder guide.
  • Corrected LICENSE link typo.
  • +2/-2     

    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 April 15, 2025 19:35
    @Vinocis Vinocis self-assigned this Apr 15, 2025
    @github-actions
    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
    ⚡ Recommended focus areas for review

    Error Handling

    The new code introduces multiple error handling paths for invalid metadata. Ensure that these error messages are consistent and cover all possible invalid input scenarios.

    @spec build(metadata()) :: {:ok, %{script: bitstring()}} | {:error, bitstring()}
    def build(%{splits: _splits} = metadata) do
      with {:ok, {port_splits, fixed_splits}} <- check_metadata(metadata) do
        remaining_dest = metadata[:remaining_to]
        asset = metadata[:percent_asset]
    
        with {:ok, port_numscript} <-
               maybe_build_portioned_numscript(port_splits, remaining_dest, asset),
             {:ok, fixed_numscript} <- maybe_build_fixed_values_numscript(fixed_splits) do
          {:ok, %{script: port_numscript <> fixed_numscript}}
        end
      end
    end
    
    def build(_metadata), do: {:error, "Missing key on metadata."}
    Validation Logic

    The validation logic for percent_asset, remaining_to, and account fields has been added. Verify that these validations are comprehensive and correctly implemented.

      case {String.trim(asset), String.trim(dest)} do
        {_, ""} ->
          {:error, "Invalid destination."}
    
        {"", _} ->
          {:error, "Invalid asset."}
    
        {asset, dest} ->
          """
          send [#{asset} #{amount}] (
            source = @user
            destination = @#{dest}
          )
          """
      end
    end
    
    defp build_numscript(%{amount: amount, account: dest, type: type})
         when type == :percent and
                is_integer(amount) and
                amount > 0 and
                amount <= 100 and
                is_binary(dest) do
      case String.trim(dest) do
        "" ->
          {:error, "Invalid destination."}

    @github-actions
    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Validate script initialization

    Ensure that the script variable is properly initialized before appending to it to
    avoid potential runtime errors.

    lib/numscriptex/builder.ex [218-228]

    -defp close_numscript(script, nil) do
    +defp close_numscript(script, nil) when is_binary(script) do
     script =
       script <>
         """
             remaining kept
           }
         )
         """
     
     {:ok, script}
    Suggestion importance[1-10]: 7

    __

    Why: Adding a guard clause to ensure script is a binary before appending to it enhances robustness by preventing potential runtime errors. This is a reasonable improvement in type safety, although the existing code may already assume script is a binary.

    Medium
    Add guard clause for script

    Add a guard clause to ensure script is a binary before appending to it, to prevent
    unexpected errors.

    lib/numscriptex/builder.ex [252-256]

    -defp close_portioned_dest(script, nil) do
    +defp close_portioned_dest(script, nil) when is_binary(script) do
     script <>
       """
             remaining kept
           }
       """
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion to add a guard clause ensures that script is a binary, which can prevent unexpected errors during execution. This adds an extra layer of safety, although the existing logic might already handle this implicitly.

    Medium

    Copy link

    @cubic-dev-ai cubic-dev-ai bot left a comment

    Choose a reason for hiding this comment

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

    mrge found 1 issue across 3 files. View it in mrge.io

    Copy link
    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! Bora deploy? 🚀

    @fermuch fermuch merged commit 3ffeadc into main Apr 15, 2025
    5 checks passed
    @fermuch fermuch deleted the hotfix/builder-validations branch April 15, 2025 19:57
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    3 participants