Enforce non-standard currency rule of not starting with 0x00

[gregtatcam]

High Level Overview of Change

This amendment enforces non-standard currency rule per XRPL documentation.

• Add amendment fixNonStandardCurrency
• Fail AMMCreate, CheckCreate, OfferCreate, TrustSet, and Payment transactions if non-standard currency starts with 0x00

Type of Change

• [ ] Bug fix (non-breaking change which fixes an issue)
• [ ] New feature (non-breaking change which adds functionality)
• [x] Breaking change (fix or feature that would cause existing functionality to not work as expected)
• [ ] Refactor (non-breaking change that only restructures code)
• [ ] Performance (increase or change in throughput and/or latency)
• [x] Tests (you added tests for code that already exists, or your new feature included in this PR)
• [x] Documentation update
• [ ] Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
• [ ] Release

Test Plan

Updated unit-tests:

• AMM
• Check
• Flow
• Offer
• SetTrust

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 76.2%. Comparing base (9a6af9c) to head (4b529f1). Report is 164 commits behind head on develop.

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #5123     +/-   ##
=========================================
+ Coverage     76.1%   76.2%   +0.1%     
=========================================
  Files          760     760             
  Lines        61548   61602     +54     
  Branches      8160    8138     -22     
=========================================
+ Hits         46834   46916     +82     
+ Misses       14714   14686     -28     

Files with missing lines	Coverage Δ
include/xrpl/protocol/Feature.h	100.0% <ø> (ø)
include/xrpl/protocol/UintTypes.h	100.0% <ø> (ø)
src/libxrpl/protocol/Feature.cpp	94.6% <ø> (ø)
src/libxrpl/protocol/UintTypes.cpp	96.8% <100.0%> (+2.2%)	:arrow_up:
src/xrpld/app/tx/detail/AMMCreate.cpp	90.4% <100.0%> (+0.2%)	:arrow_up:
src/xrpld/app/tx/detail/CreateCheck.cpp	97.5% <100.0%> (+0.1%)	:arrow_up:
src/xrpld/app/tx/detail/CreateOffer.cpp	91.4% <100.0%> (+0.1%)	:arrow_up:
src/xrpld/app/tx/detail/NFTokenUtils.cpp	91.4% <100.0%> (+0.1%)	:arrow_up:
src/xrpld/app/tx/detail/Payment.cpp	86.8% <100.0%> (+0.3%)	:arrow_up:
src/xrpld/app/tx/detail/SetTrust.cpp	89.6% <100.0%> (+0.2%)	:arrow_up:

... and 7 files with indirect coverage changes

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[mvadari]

How will this work for existing tokens that already start with 0x00? Will those effectively be banned? Can the reserves in those objects be reclaimed?

[gregtatcam]

How will this work for existing tokens that already start with 0x00? Will those effectively be banned? Can the reserves in those objects be reclaimed?

I have not added yet any handling for the existing tokens starting with 0x00. There are currently 22 of those (as of couple weeks ago) and they can be white-listed. But just the currency code, not the currency/issuer combination. I'll add the list to validCurrency function.

[MarkusTeufelberger]

This list should be in client applications, not in the protocol.

[gregtatcam]

This list should be in client applications, not in the protocol.

If the white-listed currencies are in the client application and the protocol enforces the rule for the non-standard currencies then the transactions with the white-listed currencies are going to fail. The intention is to allow the non-standard currencies created pre-amendment to work.

[MarkusTeufelberger]

The protocol should not interpret currency codes at all. Otherwise you should also start checking for valid ASCII characters and even only allow ISO-4217 codes for "standard currencies" per the documentation.

[donovanhide]

For my use case, I will have to hard code this whitelist into my app, as all those listed Currencies will still be tradable, with whatever current issuers, and also any issuers in the future may choose to use them. Feels like the ship sailed and you should just live with any "bad" currency codes now and in the future rather than trying to enforce rules retrospectively.

Edit: To be clear, the reason I would have to hardcode is to match the amendment list. If someone does add a "bad" currency during the voting period, I have to mark that currency as untradable, while the whitelisted ones continue to be tradable. Total PITA :-)

[gregtatcam]

The motivation for enforcing the non-standard currency rule is efficient serialization of Multi Purpose Token (MPT) in the lending protocol and AMM. AMM is currently identified by a token pair where each token is currency (160 bits)+account (160 bits) and is serialized as 320 bits. MPT is identified by 192 bits MPTID: 32 bits sequence + 160 bits account. Without the rule enforced, MPT is going to be serialized as 160+160+32 = 352 bits. With the rule enforced, MPT is serialized as 16+192 bits, where 16 bits is a mask starting with zero byte. If the first two bytes of the serialized data matches the mask then the next 192 bits is MPTID, otherwise it's a currency code followed by account. The saving is 144 bits/18 bytes. Based on this PR feedback, there are clearly downsides/challenges to this approach, but sharing this added context to see if any other suggestions for eliminating those downsides while enabling more efficient serialization.

[donovanhide]

Are you referring to this? https://github.com/XRPLF/XRPL-Standards/tree/master/XLS-0033d-multi-purpose-tokens#a114-why-isnt-mptokenissuanceid-constructed-from-the-hash-of-an-issuer-address-and-currency-code

I can't find any reference to the 16 bit mask issue you have just raised in your previous comment. Could you please elucidate?

[gregtatcam]

Are you referring to this? https://github.com/XRPLF/XRPL-Standards/tree/master/XLS-0033d-multi-purpose-tokens#a114-why-isnt-mptokenissuanceid-constructed-from-the-hash-of-an-issuer-address-and-currency-code

I can't find any reference to the 16 bit mask issue you have just raised in your previous comment. Could you please elucidate?

Correct. This spec doesn't include MPT serialization for AMM because MPT is not integrated into DEX in V1 release.

We can serialize MPT as 0x00ff + 32 bit sequence + 160 bit account. The mask is 0x00ff. When we deserialize, we look at the first 16 bits. Standard currency starts with 0x00 but is followed by another 88 zero bit. So it can't be standard currency. If non-standard currency rule is enforced then it can't be non-standard currency since it can't start with 0x00. We infer this to be MPTID and read next 192 bits. If the first two bytes are not 0x00ff then the data must be 160 bit currency + 160 bit account.

[donovanhide]

Why not check if all the bytes are zero except the three standard currency bytes rather than just checking the first two bytes? Then you have completely validated the currency code? If so, come up with a short encoding where the actual three bytes of the standard currency code are encoded rather than the full 20 bytes. Maybe you need a prefix byte which says which of the two options you are using giving you a 16 byte saving in the standard currency code case. TBH, this all seems a bit too clever, much like using the inf bit of the float to encode both fixed and floating point numbers into an Amount.

[gregtatcam]

Why not check if all the bytes are zero except the three standard currency bytes rather than just checking the first two bytes? Then you have completely validated the currency code? If so, come up with a short encoding where the actual three bytes of the standard currency code are encoded rather than the full 20 bytes. Maybe you need a prefix byte which says which of the two options you are using giving you a 16 byte saving in the standard currency code case. TBH, this all seems a bit too clever, much like using the inf bit of the float to encode both fixed and floating point numbers into an Amount.

I'm not following. I don't need to validate anything. I need to distinguish MPT from currency+account. Current encoding for AMM token pair (STIssue type) is asset: 160 currency + 160 bit account; asset2: 160 currency + 160 bit account. MPT is 192 bit data. We need to serialize MPT so that it is backward compatible with the current STIssue serialization.

[donovanhide]

I can't really comment on unwritten specs. If you are expecting me as a consumer of rippled binary output to check the first two bytes of a STIssue and then also compare its first 20 bytes to a whitelist of 20 bytes currencies to see if is is either a MPT Issuance or a "classic" Issuance, then I think you have a bad design. Sorry.

[bthomee]

This PR is no longer needed now MPT has been merged.