Fix AMM missing XRP Asset Metadata #5012

shawnxie999 · 2024-05-02T15:30:48Z

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

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)
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 · 2024-05-02T15:59:31Z

Codecov Report

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

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

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	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%)`	⬆️
src/ripple/protocol/impl/STCurrency.cpp	`73.5% <0.0%> (-7.1%)`	⬇️

... and 5 files with indirect coverage changes

gregtatcam · 2024-05-02T18:14:24Z

src/ripple/protocol/impl/STCurrency.cpp

@@ -71,6 +73,12 @@ STCurrency::isEquivalent(const STBase& t) const
 bool
 STCurrency::isDefault() const
 {
+    if (auto const rules = getCurrentTransactionRules())


This changes semantics of this call. Post-amendment STCurrency and STIssue will not have the default value. Is this really the right behavior?

#4965 (comment)

here is what Ed said about default value

Is isDefault() used for the metadata insertion only? If not then is returning always false consistent with other uses?

Yes i believe isDefault() is only used for metadata

gregtatcam · 2024-05-02T19:03:25Z

src/test/app/Oracle_test.cpp

+        for (Json::Value const& metaNode : meta[sfAffectedNodes.jsonName])
+        {
+            // Ensure XRP asset is shown in PriceData regardless of the
+            // fixAMMMetadata amendment


Why is this not amendment guarded?

This is not a problem with Oracle I believe, since the asset is an inner object

Why it should not be amendment guarded if it's an inner object?

Inner object default values will not omitted. Metadata would only check the outer object

gregtatcam · 2024-05-02T20:07:35Z

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 · 2024-05-03T13:07:34Z

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 · 2024-05-03T13:22:38Z

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 · 2024-05-19T14:27:42Z

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 · 2024-05-30T12:09:13Z

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.

dangell7 · 2024-05-30T14:10:23Z

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 · 2024-06-03T13:41:32Z

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.

dangell7 · 2024-06-03T14:34:54Z

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 · 2024-06-03T16:55:35Z

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.

Bronek · 2024-06-05T10:39:03Z

src/ripple/protocol/Feature.h

@@ -360,6 +360,7 @@ extern uint256 const fixEmptyDID;
 extern uint256 const fixXChainRewardRounding;
 extern uint256 const fixPreviousTxnID;
 extern uint256 const fixAMMRounding;
+extern uint256 const fixAMMMetadata;


Having read #4965 (comment) I think this is a good fix, but I would like the amendment to use a different name. The use of AMM in name is misleading, this is a currency metadata fix (i.e. a lower abstraction level), which just happens to show up in AMM (but also in Oracle, as the unit tests demonstrate).

Could be also named as asset metadata, rather than currency metadata.

fix xrp metadata

2c0572c

shawnxie999 added the Amendment label May 2, 2024

shawnxie999 marked this pull request as ready for review May 2, 2024 17:21

gregtatcam reviewed May 2, 2024

View reviewed changes

shawnxie999 added the Bug label May 3, 2024

Bronek requested changes Jun 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix AMM missing XRP Asset Metadata #5012

Fix AMM missing XRP Asset Metadata #5012

shawnxie999 commented May 2, 2024 •

edited

codecov-commenter commented May 2, 2024 •

edited

gregtatcam May 2, 2024

shawnxie999 May 2, 2024

gregtatcam May 2, 2024

shawnxie999 May 3, 2024

gregtatcam May 2, 2024

shawnxie999 May 2, 2024

gregtatcam May 3, 2024

shawnxie999 May 3, 2024

gregtatcam commented May 2, 2024

shawnxie999 commented May 3, 2024

gregtatcam commented May 3, 2024

exunda commented May 19, 2024

Bronek commented May 30, 2024

dangell7 commented May 30, 2024

Bronek commented Jun 3, 2024

dangell7 commented Jun 3, 2024

Bronek commented Jun 3, 2024

Bronek Jun 5, 2024

Bronek Jun 5, 2024

Fix AMM missing XRP Asset Metadata #5012

Are you sure you want to change the base?

Fix AMM missing XRP Asset Metadata #5012

Conversation

shawnxie999 commented May 2, 2024 • edited

High Level Overview of Change

Context of Change

Type of Change

API Impact

codecov-commenter commented May 2, 2024 • edited

Codecov Report

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

gregtatcam commented May 2, 2024

shawnxie999 commented May 3, 2024

gregtatcam commented May 3, 2024

exunda commented May 19, 2024

Bronek commented May 30, 2024

dangell7 commented May 30, 2024

Bronek commented Jun 3, 2024

dangell7 commented Jun 3, 2024

Bronek commented Jun 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawnxie999 commented May 2, 2024 •

edited

codecov-commenter commented May 2, 2024 •

edited