Fix AMM missing XRP Asset Metadata

[shawnxie999]

trafficstars

High Level Overview of Change

In the NewFields of a AMM object after a AMMCreate transaction, Asset would be omitted if it is XRP, which is incorrect. A new amendment fixAMMMetadata fixes this problem

Context of Change

Type of Change

• [x] Bug fix (non-breaking change which fixes an issue)
• [ ] New feature (non-breaking change which adds functionality)
• [ ] 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)
• [ ] Documentation update
• [ ] Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
• [ ] Release

API Impact

• [ ] Public API: New feature (new methods and/or new fields)
• [ ] Public API: Breaking change (in general, breaking changes should only impact the next api_version)
• [ ] libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
• [ ] Peer protocol change (must be backward compatible or bump the peer protocol version)

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 50.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 70.9%. Comparing base (5aa1106) to head (2c0572c). Report is 249 commits behind head on develop.

Files with missing lines	Patch %	Lines
src/ripple/protocol/impl/STCurrency.cpp	0.0%	3 Missing :warning:

Additional details and impacted files

@@           Coverage Diff           @@
##           develop   #5012   +/-   ##
=======================================
  Coverage     70.9%   70.9%           
=======================================
  Files          796     796           
  Lines        66792   66798    +6     
  Branches     11002   10998    -4     
=======================================
+ Hits         47379   47385    +6     
  Misses       19413   19413           

Files with missing lines	Coverage Δ
src/ripple/protocol/Feature.h	100.0% <ø> (ø)
src/ripple/protocol/impl/Feature.cpp	93.9% <ø> (ø)
src/ripple/protocol/impl/STIssue.cpp	94.1% <100.0%> (+0.6%)	:arrow_up:
src/ripple/protocol/impl/STCurrency.cpp	73.5% <0.0%> (-7.1%)	:arrow_down:

... and 5 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.

[gregtatcam]

Does it make sense to add another virtual method to STBase, say canInsertMetadata() with the default implementation returning isDefault() and STIssue/STCurrency returning false? canInsertMetadata() is only called in the metadata handling. This ensures that isDefault() semantic is not broken.

[shawnxie999]

Does it make sense to add another virtual method to STBase, say canInsertMetadata() with the default implementation returning isDefault() and STIssue/STCurrency returning false? canInsertMetadata() is only called in the metadata handling. This ensures that isDefault() semantic is not broken.

I'm not sure if that would be needed. isDefault() itself is only used for metadata purposes(though would need to double check this), so I don't think we would need to introduce another method for this. And by Ed's definition, isDefault() is only supposed to return true if the value represents "nothing", so omitting XRP would clearly be wrong behavior.

[gregtatcam]

Does it make sense to add another virtual method to STBase, say canInsertMetadata() with the default implementation returning isDefault() and STIssue/STCurrency returning false? canInsertMetadata() is only called in the metadata handling. This ensures that isDefault() semantic is not broken.

I'm not sure if that would be needed. isDefault() itself is only used for metadata purposes(though would need to double check this), so I don't think we would need to introduce another method for this. And by Ed's definition, isDefault() is only supposed to return true if the value represents "nothing", so omitting XRP would clearly be wrong behavior.

Can you confirm it's only used for the metadata?

Does badCurrency() represent "nothing"? By the way, STAmount has isDefault() as return (mValue == 0) && mIsNative;; i.e. XRP(0), which is clearly not "nothing".

[exunda]

Does it make sense to add another virtual method to STBase, say canInsertMetadata() with the default implementation returning isDefault() and STIssue/STCurrency returning false? canInsertMetadata() is only called in the metadata handling. This ensures that isDefault() semantic is not broken.

I'm not sure if that would be needed. isDefault() itself is only used for metadata purposes(though would need to double check this), so I don't think we would need to introduce another method for this. And by Ed's definition, isDefault() is only supposed to return true if the value represents "nothing", so omitting XRP would clearly be wrong behavior.

Can you confirm it's only used for the metadata?

Does badCurrency() represent "nothing"? By the way, STAmount has isDefault() as return (mValue == 0) && mIsNative;; i.e. XRP(0), which is clearly not "nothing".

That is really a scary question for a Rippel employee to ask! 😨 The badCurrency is the encoding of the string XRP as 160-bit asset code and used to prevent trust lines with that asset code from being created, probably as a safety to prevent people from being tricked. But attackers can still do this trick by using other character case or even a space. Very sloppy. I am surprsied nobody else has discussed this!

[Bronek]

FWIW, ~I am uncomfortable with this change. Both STCurrency and STIssue are basic protocol building blocks, and because of this we cannot assume that isDefault() functions will be only ever used for metadata generation (that is, regardless of the answer to the question posed by @gregtatcam , things may change in the future). IMO we should either add a new function with the required semantics and change other bits (i.e. where metadata is generated) to use it, or do something else.~

EDIT: changed my opinion, scroll below

[dangell7]

FWIW, I am uncomfortable with this change. Both STCurrency and STIssue are basic protocol building blocks, and because of this we cannot assume that isDefault() functions will be only ever used for metadata generation (that is, regardless of the answer to the question posed by @gregtatcam , things may change in the future). IMO we should either add a new function with the required semantics and change other bits (i.e. where metadata is generated) to use it, or do something else.

I agree with this.

Here is what we did:

bool const recordDefaultAmounts = to.rules().enabled(fixXahauV1);
...
bool const shouldRecord =
                    (obj.getSType() == STI_AMOUNT && recordDefaultAmounts) ||
                    !obj.isDefault();

[Bronek]

FWIW, I am uncomfortable with this change. Both STCurrency and STIssue are basic protocol building blocks, and because of this we cannot assume that isDefault() functions will be only ever used for metadata generation (that is, regardless of the answer to the question posed by @gregtatcam , things may change in the future). IMO we should either add a new function with the required semantics and change other bits (i.e. where metadata is generated) to use it, or do something else.

I agree with this.

Here is what we did:

bool const recordDefaultAmounts = to.rules().enabled(fixXahauV1);
...
bool const shouldRecord =
                    (obj.getSType() == STI_AMOUNT && recordDefaultAmounts) ||
                    !obj.isDefault();

Having read https://github.com/XRPLF/rippled/issues/4965#issuecomment-2079688630 I think I was mistaken. The semantics of isDefault() is defined in STBase ("does the object hold any useful data ?"); neither STIssue nor STCurrency implement it correctly, as of today. Having said that, I also think that the proposed amendment is badly named, as this is not an AMM amendment. It is a currency/IOU metadata amendment.

[dangell7]

FWIW, I am uncomfortable with this change. Both STCurrency and STIssue are basic protocol building blocks, and because of this we cannot assume that isDefault() functions will be only ever used for metadata generation (that is, regardless of the answer to the question posed by @gregtatcam , things may change in the future). IMO we should either add a new function with the required semantics and change other bits (i.e. where metadata is generated) to use it, or do something else.

I agree with this. Here is what we did:

bool const recordDefaultAmounts = to.rules().enabled(fixXahauV1);
...
bool const shouldRecord =
                    (obj.getSType() == STI_AMOUNT && recordDefaultAmounts) ||
                    !obj.isDefault();

Having read #4965 (comment) I think I was mistaken. The semantics of isDefault() is defined in STBase ("does the object hold any useful data ?"); neither STIssue nor STCurrency implement it correctly, as of today. Having said that, I also think that the proposed amendment is badly named, as this is not an AMM amendment. It is a currency/IOU metadata amendment.

I disagree as STAmount clearly defines the isDefault() as return (mValue == 0) && mIsNative; so if that is incorrect then yeah it should just be false AND STAmount should be fixed as well, however if that is the correct default response then STIssue and STCurrency are correct in their response and this fix needs to be handled outside of the isDefault() behavior.

But alas I really don't have any firm position on how its handled in ripple(d).

[Bronek]

return (mValue == 0) && mIsNative;

I think the condition that you are citing here just happens to match the definition of "no useful data here", in the context of STAmount.

[donovanhide]

Hi, it would be great if this PR got pushed through, it is a major complication for dealing with newly created AMM's which haven't yet traded. Workaround at the moment means you have to scan the other metadata ledger entries for the matching AccountRoot to get the missing Asset XRP balance. Example transaction:

B2365B7E85CFB1A7F9521CB37EF206D503135C695B689AD4C553BC8E8A215634

[bthomee]

@shawnxie999 can you please rebase, resolve the conflicts, and address the open items in the comments, if any?

[shawnxie999]

@bthomee I won't be able to get to this PR for a while.