test: Add more test cases for Base58 parser

[ximinez]

High Level Overview of Change

Adds unit tests related to the AccountID handling updated in 2.2.1 (#5078)

Context of Change

2.2.1 was released without all of the relevant unit tests for reasons. This PR finally follows up on that and adds the tests.

(Also fixes an unused variable build warning on Release builds.)

Type of Change

• Refactor (non-breaking change that only restructures code)
• Tests (you added tests for code that already exists, or your new feature included in this PR)

[mvadari]

What does this line do?

[ximinez]

It fools the compiler into thinking the variable is used, so you don't get unused variable warnings when asserts are disabled.

In file included from /home/extra/ed/dev/rippled/current/src/ripple/protocol/impl/tokens.cpp:32:
/home/extra/ed/dev/rippled/current/src/ripple/protocol/impl/b58_utils.h: In function ‘std::array<unsigned char, 10> ripple::b58_fast::detail::b58_10_to_b58_be(uint64_t)’:
/home/extra/ed/dev/rippled/current/src/ripple/protocol/impl/b58_utils.h:172:29: warning: unused variable ‘B_58_10’ [-Wunused-variable]
  172 |     constexpr std::uint64_t B_58_10 = 430804206899405824;  // 58^10;
      |                             ^~~~~~~

It's just an annoyance, but it was on the branch I had sitting around, so I included it.

[vlntb]

In this case it would be better to use maybe_unused attribute:

static constexpr [[maybe_unused]] std::uint64_t B_58_10 = 430804206899405824; // 58^10

[ximinez]

Yeah, I keep forgetting that exists. LOL

[ximinez]

The final commit message should not include "NFTs". That function was an unintentional duplicate.

Also need to add @thejohnfreeman as a coauthor, since he wrote the original unit test.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.9%. Comparing base (838978b) to head (a7d0dd3).

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #5174     +/-   ##
=========================================
- Coverage     77.9%   77.9%   -0.0%     
=========================================
  Files          782     782             
  Lines        66621   66619      -2     
  Branches      8161    8136     -25     
=========================================
- Hits         51902   51899      -3     
- Misses       14719   14720      +1     

Files with missing lines	Coverage Δ
include/xrpl/protocol/detail/b58_utils.h	95.5% <ø> (-0.1%)	⬇️

... and 4 files with indirect coverage changes

[vlntb]

Very minor changes.

[vlntb]

In this case it would be better to use maybe_unused attribute:

static constexpr [[maybe_unused]] std::uint64_t B_58_10 = 430804206899405824; // 58^10

[vlntb]

[nit] For me the intention is not very clear - is this a deliberate scope introduction or a mistake in using if statement. To make it clearer I would mark the borders of the scope of if explicitly to make clearly separate for the new scope.

if (BEAST_EXPECT(parseBase58<AccountID>(s)))
{
    BEAST_EXPECT(toBase58(*parseBase58<AccountID>(s)) == s);
}

{
    auto const s =
        "âabcd1rNxp4h8apvRis6mJf9Sh8C6iRxfrDWNâabcdAVâ\xc2\x80\xc2\x8f";
    BEAST_EXPECT(!parseBase58<AccountID>(s));
}

[ximinez]

Yeah, now that you mention it, it does look a little off. I changed the test a little bit to make that all clearer, and to remove the need to parse s twice.

[ximinez]

Proposed commit message:

 test: Add more test cases for Base58 parser (#5174)

---------
Co-authored-by: John Freeman <[email protected]>

[ximinez]

Hold merging until after 2.3.0 final release.