AMMCreate transaction missing Asset field in affected nodes (Version: 2.1.0)

[Silkjaer]

Issue Description

When a new AMM is created, the created node in affected nodes miss the Asset field if it is XRP.

Steps to Reproduce

Example transaction: https://xrpscan.com/tx/BDCC35ED3EEA1F6E4AC76C21395F98239BACF9D59708CF3A4560830FF734AEF7

[shawnxie999]

This is expected behavior. The metadata would omit fields that have default values. In this case, Asset field's default value is XRP, and therefore it would be omitted. There is really no need to look into the metadata for this info - one could simply look at the transaction payload to find out the values both Amount and Amount2

[Silkjaer]

I disagree, as other objects do not omit default values in the metadata when they are created.

[joynancy]

The amount and amount2 values are not omitted, they are included in preload transaction fields. The other explorers are displaying the assets and amounts without any issue. e.g. https://bithomp.com/explorer/BDCC35ED3EEA1F6E4AC76C21395F98239BACF9D59708CF3A4560830FF734AEF7; https://livenet.xrpl.org/transactions/BDCC35ED3EEA1F6E4AC76C21395F98239BACF9D59708CF3A4560830FF734AEF7

I am reaching out to XRPScan team to see if they agree to update.

[shawnxie999]

@Silkjaer Other objects do have default values that are omitted (like empty array), but they might be less obvious. I do agree with you that it would be more clear if XRP is not omitted. However, I don't think it would cause information loss, as the user could retrieve the AMM data in other ways. In the case of AMM metadata, they could look at what the submitter entered for the transaction payload for the asset amounts, which is more efficient than looking at the metadata. I'm open to oppositions.

[Silkjaer]

The purpose is not accessing the data, or figuring out a different way to do so, but it's about the expected behavior, such that e.g. tracking objects and their states can be done consistently regardless of type.

I.e. I am scanning all objects, their states and their changes by monitoring affected nodes of all transactions storing it elsewhere. I have had to make an exception for this object type now.

I have not come across other objects that have a non-empty default state for a property, which is not published in the Affected Nodes when the object is created.

[ximinez]

In the case of AMM metadata, they could look at what the submitter entered for the transaction payload for the asset amounts, which is more efficient than looking at the metadata. I'm open to oppositions.

@shawnxie999 As a general rule, we want to discourage looking at the transaction payload as a source of truth. That's how we got things like the partial payment exploit. The metadata is the canonical truth about what happened during a transaction, so it should be accurate and complete enough that there is no need to look at the transaction payload.

Now that I'm looking into this, a little background: sfAsset is an STIssue (serialized as STI_ISSUE). STIssue is a new type introduced with the AMM functionality / amendment. That class defines an isDefault function:

bool
STIssue::isDefault() const
{
    return issue_ == xrpIssue();
}

However, if you look at the isDefault functions for almost every other ST* class, isDefault checks for some kind of "nothing" - a zero value, an empty object, or an uninitialized state. (The exception is STCurrency, which was introduced after STIssue, and probably followed its precedent.)

isDefault is not intended to check "is this currently holding the class's default value?". It is intended to check "is this holding nothing useful?". For most classes, the default value and "nothing" are basically the same, so I can understand the confusion. Admittedly, this is not really well documented either.

More importantly, XRP is not "nothing".

I agree with @Silkjaer - This behavior is very clearly wrong.

I propose that this needs to be fixed, either by:

1. changing both STIssue::isDefault() and STCurrency::isDefault() to always return false. (And add an explanation to STBase::isDefault() so that this doesn't happen again.)
2. changing STIssue and STCurrency to store a std::optional as the data member.

My vote is for option 1. Either way, it'll need an amendment.

[shawnxie999]

@ximinez how would an amendment for changing the behavior of a ST* class work? Has there been a precedence for this? I am not not really sure if this needs an amendment because it only impacts the metadata and is not transaction breaking

[ximinez]

@shawnxie999 We've done it a couple of times. Look for the STAmountSO and NumberSO classes.

Luckily, we just merged in a change to make a global Rules object available during transaction processing (which is the only time isDefault is relevant). The isDefault functions could check that Rules object, if available to change their behavior.

This is absolutely transaction breaking. It doesn't (significantly) change the processing, but it does change what is stored in the node store.

[joynancy]

Thanks to the XRPScan project team for the quick fix. The amount2 field is now added to the XRPScan UI. See example: https://xrpscan.com/tx/3448E54EA7885B59CD4FFE85814F6EF3BDC500E9C4273895CA829D87D8CFEC1A