Open Silkjaer opened 6 months ago
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
I disagree, as other objects do not omit default values in the metadata when they are created.
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.
@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.
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.
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:
STIssue::isDefault()
and STCurrency::isDefault()
to always return false
. (And add an explanation to STBase::isDefault()
so that this doesn't happen again.)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.
@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
@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.
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
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