Enforce non-standard currency rule of not starting with 0x00 #5123
Conversation
* Add amendment fixNonStandardCurrency * Fail AMMCreate, CheckCreate, OfferCreate, TrustSet, and Payment transactions if non-standard currency starts with 0x00
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
... and 7 files with indirect coverage changes |
|
How will this work for existing tokens that already start with |
I have not added yet any handling for the existing tokens starting with |
* Fail NFTokenCreateOffer, NFTokenMint for invalid non-standard currency * Allow Payment for white-listed invalid non-standard currency
| Currency{"00000000CA95E0B2A0E0B2BFE1B4A5E0B2A0CA94"}, | ||
| Currency{"000000282D5F282D5F282D5F2D295F2D295F2D29"}, | ||
| Currency{"0000000000000000000000000000000078415049"}, | ||
| Currency{"00000000000028E295ADE0B2B05FE280A2CC8129"}}; |
There was a problem hiding this comment.
An interesting thing might happen if someone creates a new invalid currency during the voting period of this amendment. Even worse if it happens during the two-week period after voting has completed. I do not think there's anything we can do about it, other than allow errors to happen on transactions with such an invalid currency in the future - I think there should be no future PR to extend this list.
There was a problem hiding this comment.
Yes, I agree. There should be no future PR to extend the list. But it might make sense to check if there are more invalid currencies created before this PR is merged. This list is good as of couple of weeks ago.
There was a problem hiding this comment.
I think the corollary from this is that in this PR we need to pay attention to enforce the rule across all possible transaction types (and also to pay attention to it in other work in progress), since it would be really "annoying" if there is found to be a transaction type that can succeed for such currencies, while other transaction types fail.
There was a problem hiding this comment.
It would be trivial to create thousands of additional entries for this list until the PR is merged. The way currency codes are being interpreted is up to client applications and should not be part of the protocol.
See also the documentation: "To prevent this from being treated as a "standard" currency code" - there is nothing stating that "standard currency codes" are a rule.
If you want to enforce standards, you could also start blocking non-ISO-4217 currency codes. This would include BTC, ETH and XRP by the way.
There was a problem hiding this comment.
There are about 140,000 currencies codes out of which ~4,700 are standard and ~135,300 are non-standard. The 22 non-standard currencies that don't follow the rule were clearly created unintentionally. Creating thousands more doesn't serve any purpose and is a violation of the rule.
There was a problem hiding this comment.
This is a guideline for interpreting a 160 bit field when using some APIs, not a rule. I also don't see any reason or benefit of limiting this in the protocol (especially with such a clunky allowlist approach, considering that there are more than one ledger out there using this software).
There was a problem hiding this comment.
What if, instead of a hard-coded whitelist, you put the whitelist on the ledger in a new singleton object that just holds an array of Currencies, much like the Amendments singleton?
It could use the LedgerFix transaction introduced in #4945 with a new LedgerFixType. Add fields to LedgerFix to specify the currency code, and one account that has an existing trust line (thus ensuring that this is a currency that already exists on the ledger, and nobody can whitelist a currency preemptively). Use the default base fee, since this isn't doing a lot of work. You've already got an amendment gating all of this, so you wouldn't need another one. It might be worth reusing the ideas from Rules so that the object doesn't have to keep being loaded, but this doesn't have to do the extra work that Rules does to handle presets, so the existing object caches may be enough - it isn't likely to expire from the cache since so many transactions will read it.
This would
- Get rid of the hard-coded list.
- Allow the whitelist to work across all networks.
- Allow any new invalid currencies created before the amendment is enabled to also be whitelisted.
- Keeping it cheap should encourage issuers to pay the few drops to fix their currencies, but if they're not, anyone can do it. Whitelisting all the current ones would only cost 200 drops total.
There was a problem hiding this comment.
It's a good idea, but it looks like we will go with the longer serialization of STIssue in order to include MPT issuances on a shorter timeline with more community buy-in. Once that happens, we will likely never again need to fix the currency serialization.
|
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. |
|
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. |
|
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 :-) |
|
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. |
|
Are you referring to this? 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. |
|
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. |
|
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. |
High Level Overview of Change
This amendment enforces non-standard currency rule per XRPL documentation.
Type of Change
.gitignore, formatting, dropping support for older tooling)Test Plan
Updated unit-tests: