Update Base58 codec and remove Bitcoin support: #3575

a-noni-mousse · 2020-08-09T00:19:14Z

Use C++17 constant expressions to calculate the inverse
alphabet map at compile time instead of at runtime.

Remove support for encoding & decoding tokens using the
Bitcoin alphabet.

Use C++17 constant expressions to calculate the inverse alphabet map at compile time instead of at runtime. Remove support for encoding & decoding tokens using the Bitcoin alphabet.

MarkusTeufelberger · 2020-08-09T08:41:46Z

src/ripple/protocol/impl/tokens.cpp

@@ -30,13 +30,22 @@

 namespace ripple {

-static char rippleAlphabet[] =
+static constexpr char const* const xrplAlphabetForward =


Suggested change

static constexpr char const* const xrplAlphabetForward =

static constexpr char const* const rippleAlphabetForward =

Also in other places...

Can you explain what you mean here? I am not understanding. You think it should use ripple? But the name 'ripple' is belong to the company and not the software.

The software is called rippled, the company is called "Ripple Labs" and it just holds the copyright for the name "Ripple" (the project named "Ripple" is much older than that). Heck, even that folder is src/ripple/.... Previous code used rippleAlphabet, changing it to xrplAlphabet just puts the ISO-4217 shorthand of a cryptocurrency in the name for no good reason.

Aha
Yes I understand you now and I prefer the name ripple also but the company is using that now because it is not RL any more and so to continue use of ripple for naming seems causes a lot of confusion.
Maybe the company can answer on what this should be because the name get used again elsewhere.

The company is actually still called "Ripple Labs Inc", it just uses the name "Ripple" in its branding. See for example https://ripple.com/terms-of-use/ ("These Terms of Use (“Terms”) apply to your access to, and use of, the websites of Ripple Labs Inc. and its subsidiaries and affiliated companies (“Ripple,” “we” or “us”), including www.ripple.com (collectively, the “Site”)."), in their privacy policy you also see the full company name (something you should maybe be aware of, if you are impersonating their CEO... ;-) ).

I guess someone working there will eventually review and resolve this.

I think we all like the name 'ripple' for this wonderful, collaborative project and feel an attachment to it. And if it were up to me I'd have kept it for the project and this could stay rippleAlphabet. But, feelings aside, there's reality. And, as has been pointed out, the naming can cause confusion.

So, since I work "there", let me play Solomon and split the proverbial baby so that we can all move forward: how about simply calling these variables alphabet and reverseAlphabet and avoiding this whole situation?

Rationale: your car doesn't use "FordNuts" or "FordBolts". It just uses "nuts" and "bolts".

I changed variable names to 'alphabetForward' and 'alphabetReverse' as you have asked

Maybe Ripple can see the problem the name 'Xrp Ledger' has and can allow the name 'ripple' to be used again in future?

MarkusTeufelberger · 2020-08-10T08:57:50Z

src/test/rpc/AccountCurrencies_test.cpp

-                "json",
-                "account_currencies",
-                boost::lexical_cast<std::string>(params))[jss::result];
-            BEAST_EXPECT(result[jss::error] == "actBitcoin");


Probably the actBitcoin Error can be removed too then.

Good spotting! Yes, I think removing rpcACT_BITCOIN from ErrorCodes.h and ErrorCodes.cpp is a good idea. If you do this, please include a comment that value 34 is unused in ErrorCodes.h. Thanks.

HowardHinnant

Looks good to me. The only thing that needs to be done that I can see is to amend the second commit message to begin with [FOLD] so that we remember to squash these two commits prior to merge.

scottschurr

Nicely done! I really like the constexpr calculation for alphabetReverse. I left some comments for minor improvements, none of which are mandatory.

I also think @HowardHinnant's request that any commits addressing review comments be marked with [FOLD] is a very good idea. The person that puts together a beta may not have been in on the review, so they don't automatically know which commits should be squashed and which should not.

Thanks.

scottschurr · 2020-08-12T01:10:26Z

src/ripple/protocol/impl/tokens.cpp

 {
-    std::string const ret = decodeBase58(s, inv);
+    assert(size != 0);


The assert is a nice addition. However, since the assert is only effective in a debug build I'd prefer replacing the assert with:

if (size == 0) return {};

If you like including the assert it could go inside the conditional. Then you'd have the assert and still have safety in a release build.

That would change output of functoin I think and didn't wnat to change behaviour that but having the assert makes sense to me. I can do this if the change is not the problem.

scottschurr · 2020-08-12T01:11:41Z

src/ripple/protocol/impl/tokens.cpp

+    // Lay the data out as
+    //      <type><token><checksum>
+    buf[0] = safe_cast<std::underlying_type_t<TokenType>>(type);
+    if (size)


If you return on size == 0 at the top of the function then you can remove this check for if (size).

scottschurr · 2020-08-12T01:19:27Z

src/ripple/protocol/impl/tokens.cpp

+static constexpr std::array<int, 256> const alphabetReverse = []() constexpr
+{
+    std::array<int, 256> map{};

-//------------------------------------------------------------------------------
+    for (auto& m : map)
+        m = -1;
+
+    for (int i = 0, j = 0; alphabetForward[i] != 0; ++i)
+        map[static_cast<unsigned char>(alphabetForward[i])] = j++;
+
+    return map;
+}


Very nicely done! Thanks.

scottschurr · 2020-08-12T01:33:40Z

src/test/rpc/AccountCurrencies_test.cpp

-                "json",
-                "account_currencies",
-                boost::lexical_cast<std::string>(params))[jss::result];
-            BEAST_EXPECT(result[jss::error] == "actBitcoin");


Good spotting! Yes, I think removing rpcACT_BITCOIN from ErrorCodes.h and ErrorCodes.cpp is a good idea. If you do this, please include a comment that value 34 is unused in ErrorCodes.h. Thanks.

scottschurr · 2020-08-12T16:36:56Z

src/ripple/protocol/impl/tokens.cpp

@@ -30,13 +30,22 @@

 namespace ripple {

-static char rippleAlphabet[] =
+static constexpr char const* const alphabetForward =


Not something I feel strongly about, but the const that follows * is redundant since alphabetForward is constexpr. So, if you want, you can omit the second const.

OK that looks good

scottschurr · 2020-08-12T16:40:58Z

src/ripple/protocol/impl/tokens.cpp

    "rpshnaf39wBUDNEGHJKLM4PQRST7VWXYZ2bcdeCg65jkm8oFqi1tuvAxyz";

-static char bitcoinAlphabet[] =
-    "123456789ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz";
+static constexpr std::array<int, 256> const alphabetReverse = []() constexpr


Another point I don't feel strongly about, but the trailing constexpr is unnecessary. As of C++17 any lambda that can be constexpr automatically is. To quote from cppreference:

constexpr: explicitly specifies that the function call operator is a constexpr function. When this specifier is not present, the function call operator will be constexpr anyway, if it happens to satisfy all constexpr function requirements

https://en.cppreference.com/w/cpp/language/lambda

Yes i didn't know that change! Very nice.

a-noni-mousse · 2020-08-12T21:24:34Z

Added the fold message and responded to review comments. ccan you reopen please?

nbougalis · 2020-08-12T21:34:48Z

I don't think we can reopen because you deleted your branch and now GitHub is confused. Please feel free to open a new PR for this! Thanks.

Update Base58 codec and remove Bitcoin support:

9b65e0c

Use C++17 constant expressions to calculate the inverse alphabet map at compile time instead of at runtime. Remove support for encoding & decoding tokens using the Bitcoin alphabet.

MarkusTeufelberger reviewed Aug 9, 2020

View reviewed changes

MarkusTeufelberger reviewed Aug 10, 2020

View reviewed changes

nbougalis requested review from HowardHinnant and scottschurr August 10, 2020 17:50

nbougalis assigned HowardHinnant and scottschurr Aug 10, 2020

Update variable names

e45ae68

HowardHinnant suggested changes Aug 11, 2020

View reviewed changes

scottschurr approved these changes Aug 12, 2020

View reviewed changes

a-noni-mousse closed this Aug 12, 2020

a-noni-mousse deleted the base58 branch August 12, 2020 21:05

a-noni-mousse mentioned this pull request Aug 12, 2020

Simplify the SetSignerList transactor #3579

Closed

a-noni-mousse mentioned this pull request Aug 12, 2020

Update Base58 codec and remove Bitcoin support: #3581

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Base58 codec and remove Bitcoin support: #3575

Update Base58 codec and remove Bitcoin support: #3575

a-noni-mousse commented Aug 9, 2020

MarkusTeufelberger Aug 9, 2020

MarkusTeufelberger Aug 9, 2020

a-noni-mousse Aug 9, 2020

MarkusTeufelberger Aug 9, 2020

a-noni-mousse Aug 9, 2020 •

edited

Loading

MarkusTeufelberger Aug 10, 2020

nbougalis Aug 10, 2020

a-noni-mousse Aug 10, 2020

MarkusTeufelberger Aug 10, 2020

scottschurr Aug 12, 2020

HowardHinnant left a comment

scottschurr left a comment

scottschurr Aug 12, 2020

a-noni-mousse Aug 12, 2020

scottschurr Aug 12, 2020

scottschurr Aug 12, 2020

scottschurr Aug 12, 2020

scottschurr Aug 12, 2020

a-noni-mousse Aug 12, 2020

scottschurr Aug 12, 2020

a-noni-mousse Aug 12, 2020

a-noni-mousse commented Aug 12, 2020

nbougalis commented Aug 12, 2020

	static constexpr char const* const xrplAlphabetForward =
	static constexpr char const* const rippleAlphabetForward =

Update Base58 codec and remove Bitcoin support: #3575

Update Base58 codec and remove Bitcoin support: #3575

Conversation

a-noni-mousse commented Aug 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-noni-mousse Aug 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HowardHinnant left a comment

Choose a reason for hiding this comment

scottschurr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-noni-mousse commented Aug 12, 2020

nbougalis commented Aug 12, 2020

a-noni-mousse Aug 9, 2020 •

edited

Loading