refactor: Remove unused and fragile string interface from arith_uint256 #28924

1. 
   maflcko commented at 4:41 pm on November 21, 2023: member

   The string interface (base_uint(const std::string&), as well as base_uint::SetHex) is problematic for many reasons:

   • It is unused (except in test-only code).
   • It is redundant with the uint256 string interface: std::string -> uint256 -> UintToArith256.
   • It is brittle, because it inherits the brittle uint256 string interface, which is brittle due to the use of c_str() (embedded null will be treated as end-of string), etc …

   Instead of fixing the interface, remove it since it is unused and redundant with UintToArith256.

2. refactor: Remove unused and fragile string interface from arith_uint256 facf629ce8
3. test: Add uint256 string parse tests fa63f16018
4. 
   DrahtBot commented at 4:41 pm on November 21, 2023: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	TheCharlatan, ajtowns

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

5. DrahtBot renamed this:
   refactor: Remove unused and fragile string interface from arith_uint256
   refactor: Remove unused and fragile string interface from arith_uint256
   on Nov 21, 2023
6. DrahtBot added the label Refactoring on Nov 21, 2023
7. in src/test/arith_uint256_tests.cpp:98 in fa63f16018

   95@@ -95,25 +96,25 @@ BOOST_AUTO_TEST_CASE( basics ) // constructors, equality, inequality
   96     BOOST_CHECK(ZeroL == (OneL << 256));
   97 
   98     // String Constructor and Copy Constructor
   

   ---

   ---

   

   TheCharlatan commented at 9:39 am on November 23, 2023:

   This comment should be adjusted, since there is no arith_uint256(const std::string& str) constructor anymore.

   ---

   

   maflcko commented at 10:04 am on November 23, 2023:

   I think it is fine to call construction from a string “string constructor”, but may change if I have to re-touch again for other reasons.
8. in src/test/uint256_tests.cpp:282 in fa63f16018

   278@@ -279,6 +279,34 @@ BOOST_AUTO_TEST_CASE( operator_with_self )
   279     BOOST_CHECK(v == UintToArith256(uint256S("0")));
   280 }
   281 
   282+BOOST_AUTO_TEST_CASE(parse)
   

   ---

   ---

   

   TheCharlatan commented at 9:52 am on November 23, 2023:

   Nit: Put these extra tests in a separate commit?

   ---

   

   maflcko commented at 9:59 am on November 23, 2023:

   Yes, it is a separate commit?

   ---

   

   TheCharlatan commented at 10:01 am on November 23, 2023:

   ^^ I ran HEAD~2 by mistake when reviewing this.
9. in src/test/arith_uint256_tests.cpp:107 in fa63f16018

   111+    BOOST_CHECK(arith_uint256S("0x" + OneL.ToString()) == OneL);
   112+    BOOST_CHECK(arith_uint256S("0x" + MaxL.ToString()) == MaxL);
   113+    BOOST_CHECK(arith_uint256S(R1L.ToString()) == R1L);
   114+    BOOST_CHECK(arith_uint256S("   0x" + R1L.ToString() + "   ") == R1L);
   115+    BOOST_CHECK(arith_uint256S("") == ZeroL);
   116+    BOOST_CHECK(R1L == arith_uint256S(R1ArrayHex));
   

   ---

   ---

   

   TheCharlatan commented at 9:55 am on November 23, 2023:

   These same tests also exist in uint256_tests. What are these additionally testing now? Can they be removed?

   ---

   

   maflcko commented at 10:02 am on November 23, 2023:

   There are two files. arith_uint256_tests.cpp, which mostly tests the arith* class, and uint256_tests.cpp, which mostly tests the uint* class.

   ---

   

   TheCharlatan commented at 10:14 am on November 23, 2023:

   They’re not really testing arith_uint256 anymore though, just the new conversion helper. I don’t feel strongly about this, these are just a bunch of test lines after all, but was curious what the criteria was here.

   ---

   

   maflcko commented at 10:37 am on November 23, 2023:

   Yeah, this line is checking the arith_uint::operator==, or maybe a test self-sanity-check? Seems fine to keep when in doubt, I guess.
10. TheCharlatan approved
11. 
    TheCharlatan commented at 10:19 am on November 23, 2023: contributor

    ACK fa63f16018d9468e1751d2107b5102184ac2d5ae
12. 
    ajtowns commented at 2:59 pm on December 7, 2023: contributor

    ACK fa63f16018d9468e1751d2107b5102184ac2d5ae
13. fanquake merged this on Dec 7, 2023
14. fanquake closed this on Dec 7, 2023

    ---

15. maflcko deleted the branch on Jan 3, 2024

Contributors
maflcko DrahtBot TheCharlatan ajtowns

Labels
Refactoring