Fix the rendering issues of some XDR fields #2876

hidenori-shinohara · 2021-01-13T18:42:07Z

Description

Resolves #2869

The "value0" issue. This seems like a feature rather than a bug (for instance, see this cereal issue ) Long story short, it's there to support multi-entry outputs such as

{          
    "value0": {
        "x": 1,
        "y": 2,
        "z": 3
    },    
    "value1": {           
        "x": 4,                
        "y": 5,
        "z": 6
    },
    "value2": {
        "x": 7,
        "y": 8,
        "z": 9
    }
}

xdr_to_string has an optional field name argument. When that's not passed, Cereal uses the word "value0". This PR will make the argument mandatory, so we will print a descriptive name given by a caller, instead of "value0". We already use this in multiple places, and this PR updates the usage of xdr_to_string when the field name argument is not passed.
I used the following XDR to make sure that the asset list is rendered correctly. With this PR, the XDR will be rendered as following:

...                             "destAsset": "XLM",
                                "destAmount": 2000000000,
                                "path": [
                                    "CENTUS",
                                    "USD"
                                ]
...

instead of

...
                                "path": [
                                    {
                                        "type": "ASSET_TYPE_CREDIT_ALPHANUM12",
                                        "alphaNum12": {
                                            "assetCode": "43454e545553000000000000",
                                            "issuer": "GCD5VCGHNLC46D5O2SAZCFFP3ZF7JRBQZGSFINXWNY5HCEOE42PH3XH5"
                                        }
                                    },
                                    {
                                        "type": "ASSET_TYPE_CREDIT_ALPHANUM4",
                                        "alphaNum4": {
                                            "assetCode": "55534400",
                                            "issuer": "GDJAG52BJUFI6RBKFTNUEJBDZODJPZBLIHN3SXMR7QIXNRAOJK3XL6P3"
                                        }
                                    }
                                ]
...

AAAAAgAAAACJCjEa69Jmm5xInlCSLmzEAf8u1q/AeXUbhoZQFHOsoQAAAGQAAAAcvpkaFAAAAAEAAAAAAAAAAAAAAAAAAAAAAAAAAQAAABx0aGlzIGlzIGEgbWVtbyBmb3IgZGVidWdnaW5nAAAAAQAAAAAAAAACAAAAAAAAAAA7msoAAAAAANhWYalldqwqmK2G0p/IIaS+64sMI9nfPeBe5RlS2NDCAAAAAAAAAAB3NZQAAAAAAgAAAAJDRU5UVVMAAAAAAAAAAAAAh9qIx2rFzw+u1IGRFK/eS/TEMMmkVDb2bjpxEcTmnn0AAAABVVNEAAAAAADSA3dBTQqPRCos20IkI8uGl+QrQdu5XZH8EXbEDkq3dQAAAAAAAAAA

Checklist

Reviewed the contributing document
Rebased on top of master (no merge commits)
Ran clang-format v5.0.0 (via make format or the Visual Studio extension)
Compiles
Ran all tests
If change impacts performance, include supporting evidence per the performance document

jonjove · 2021-01-13T19:02:43Z

Regarding the second part of your change: generally I would want to see both the asset code and the issuer.

marta-lokhova · 2021-01-13T21:10:42Z

src/catchup/simulation/TxSimApplyTransactionsWork.cpp

@@ -67,8 +67,8 @@ checkOperationResults(xdr::xvector<OperationResult> const& expected,
        if (expected[i].code() != actual[i].code())
        {
            CLOG_ERROR(History, "Expected operation result {} but got {}",
-                       xdr_to_string(expected[i].code()),
-                       xdr_to_string(actual[i].code()));
+                       xdr_to_string(expected[i].code(), "OperationResultCode"),


Now that we're printing OperationResultCode, you can change the log to be Expected {} but got {}

marta-lokhova · 2021-01-13T21:11:59Z

src/catchup/simulation/TxSimApplyTransactionsWork.cpp

@@ -181,7 +182,8 @@ checkResults(Application& app, uint32_t ledger,
            CLOG_ERROR(
                History,
                "Expected result code {} does not agree with {} for tx {}",
-                xdr_to_string(archiveRes.code()), xdr_to_string(dbRes.code()),
+                xdr_to_string(archiveRes.code(), "TransactionResultCode"),


"result code" is now redundant, change to "Expected {} does not agree with {} for tx {}"

marta-lokhova · 2021-01-13T21:14:18Z

src/catchup/simulation/TxSimApplyTransactionsWork.cpp

-                       xdr_to_string(actualOpRes));
+            CLOG_ERROR(
+                History, "{}",
+                xdr_to_string(expectedOpRes, "Expected operation result"));


nit: it's a little clearer if you make the name an actual XDR type (so OperationResult) and move "expected/actual":
CLOG_ERROR(History, "Expected {}", xdr_to_string(expectedOpRes, "OperationResult"));

makes sense!

marta-lokhova · 2021-01-13T21:15:03Z

src/catchup/simulation/TxSimApplyTransactionsWork.cpp

@@ -196,8 +198,10 @@ checkResults(Application& app, uint32_t ledger,
                    "Expected result code {} does not agree with {} for "


Same thing about "result code"

hidenori-shinohara · 2021-01-13T23:19:15Z

Updated the way Asset is handled, and now it'll be printed with both assetCode and issuer as following:

                            "pathPaymentStrictReceiveOp": {
                                "sendAsset": {   
                                    "assetCode": "XLM",           
                                    "issuer": "SDF"                                                                                                
                                },                                                                                                                 
                                "sendMax": 1000000000,          
                                "destination": "GDMFMYNJMV3KYKUYVWDNFH6IEGSL524LBQR5TXZ54BPOKGKS3DIMFIAB",
                                "destAsset": {                                                                                                     
                                    "assetCode": "XLM",
                                    "issuer": "SDF"
                                },
                                "destAmount": 2000000000,
                                "path": [
                                    {   
                                        "assetCode": "CENTUS",
                                        "issuer": "GCD5VCGHNLC46D5O2SAZCFFP3ZF7JRBQZGSFINXWNY5HCEOE42PH3XH5"
                                    },                                  
                                    {            
                                        "assetCode": "USD",    
                                        "issuer": "GDJAG52BJUFI6RBKFTNUEJBDZODJPZBLIHN3SXMR7QIXNRAOJK3XL6P3"
                                    }
                                ]       
                            }

src/util/XDRCereal.h

jonjove · 2021-01-15T21:49:54Z

src/util/XDRCereal.h

@@ -51,6 +51,27 @@ cereal_override(cereal::JSONOutputArchive& ar,
    xdr::archive(ar, tmp, field);
 }

+template <uint32_t N>
+void
+cereal_override(cereal::JSONOutputArchive& ar,


I don't think this is necessary once xdrpp/xdrpp#25 is merged.

After several attempts, I think I'll give up fixing this in a more general way such as a new save function in XDRPP or cereal_override for xvector. While I definitely think that it's suboptimal that we need to implement something like this whenever we have an xvector containing elements with cereal_override, it seems that this is our second time, so I hope that this is not that bad. Let me know what you think!

hidenori-shinohara · 2021-02-11T16:23:05Z

Updated the change using clang-format-8

jonjove · 2021-02-11T20:02:09Z

src/util/XDRCereal.cpp

-    xdr::archive(ar, stellar::assetToString(s), field);
+    xdr::archive(
+        ar,
+        std::make_tuple(


This should be changed to render native asset as "native" (no sub-object).

I only know one way to print an xvector of asset and that is to create a vector where each element is an NVP of the corresponding element of xvector. In other words, I'm not sure how I can render native asset without a sub-object and other assets as a sub-object. I made a change to make it clear that native asset is a native asset and there is no assetCode and issuer like following:

"pathPaymentStrictReceiveOp": { "sendAsset": { "assetType": "native", "assetCode/issuer": "N/A" }, ...

Let me know what you think!

Thank you for jonjove@d7472a0! I would have never come up with this idea.

I updated the PR with the change. I also tested nested containers (which we don't have now, but for future reference & out of curiosity), and it seems to work exactly as we'd expect. hidenori-shinohara/xdrpp@abe8047

This is how asset information is printed with this change:

"body": { "type": "PATH_PAYMENT_STRICT_RECEIVE", "pathPaymentStrictReceiveOp": { "sendAsset": "NATIVE", "sendMax": 1000000000, "destination": "GDMFMYNJMV3KYKUYVWDNFH6IEGSL524LBQR5TXZ54BPOKGKS3DIMFIAB", "destAsset": "NATIVE", "destAmount": 2000000000, "path": [ { "assetCode": "CENTUS", "issuer": "GCD5VCGHNLC46D5O2SAZCFFP3ZF7JRBQZGSFINXWNY5HCEOE42PH3XH5" }, { "assetCode": "USD", "issuer": "GDJAG52BJUFI6RBKFTNUEJBDZODJPZBLIHN3SXMR7QIXNRAOJK3XL6P3" } ] } }

jonjove · 2021-02-11T20:02:50Z

src/util/XDRCereal.cpp

-                    ? "SDF"
-                    : stellar::KeyUtils::toStrKey(stellar::getIssuer(asset)))),
-        field);
+    return std::make_tuple(


Same thing as mentioned in the previous commit about rendering native asset.

jonjove · 2021-02-11T20:05:07Z

src/util/XDRCereal.h

@@ -51,6 +51,22 @@ cereal_override(cereal::JSONOutputArchive& ar,
    xdr::archive(ar, tmp, field);
 }

+typedef cereal::NameValuePair<std::string> CerealStringNVP;
+typedef std::tuple<CerealStringNVP, CerealStringNVP> NVPTuple;


This is probably not a great name for this type. In general, this will only work for two NVPs but what about the case where you have 3 or more?

Makes sense. Since NVPTuple is only used a few times and it's unclear if we'll use it much more often, I got rid of the typedef.

jonjove · 2021-02-11T20:56:13Z

src/invariant/LiabilitiesMatchOffers.cpp

-                    "Balance not compatible with liabilities for account {}",
-                    xdr_to_string(account));
+                    "Balance not compatible with liabilities for {}",
+                    xdr_to_string(account, "account"));


Nitpick: Maybe "AccountEntry" for consistency?

jonjove · 2021-02-11T20:58:00Z

src/test/FuzzerImpl.cpp

+                                           "while fuzzing: {}"),
+                                txFramePtr->getResultCode(),
+                                xdr_to_string(txFramePtr->getResult(),
+                                              "transaction result"));


Nitpick: Maybe "TransactionResult"? (I know that's not exactly what it said before, but given that this is targeted at developers I think it will be clearer)

jonjove · 2021-02-11T20:58:46Z

src/transactions/OperationFrame.cpp

    }
    res = checkValid(signatureChecker, ltx, true);
    if (res)
    {
        res = doApply(ltx);
        if (Logging::logTrace("Tx"))
        {
-            CLOG_TRACE(Tx, "Operation result: {}", xdr_to_string(mResult));
+            CLOG_TRACE(Tx, "{}", xdr_to_string(mResult, "Operation result"));


Nitpick: maybe "OperationResult"?

hidenori-shinohara · 2021-02-22T21:31:20Z

I updated the PR with the cereal_override for all containers, and I also addressed all the comments.

jonjove

A couple small comments here about making the comments a bit more helpful to future maintainers. Generally, I'm happy with this. @graydon, how do you feel about the documentation level here?

jonjove · 2021-02-22T21:41:40Z

src/util/XDRCereal.h

@@ -124,7 +124,7 @@ xdr_to_string(const T& t, std::string const& name, bool compact = false)
        cereal::JSONOutputArchive ar(
            os, compact ? cereal::JSONOutputArchive::Options::NoIndent()
                        : cereal::JSONOutputArchive::Options::Default());
-        ar(cereal::make_nvp(name, t));
+        xdr::archive(ar, t, name.c_str());


I am pretty sure it's okay, but I just want to confirm that this does the right thing if name.empty() right?

Thanks for pointing that out! I didn't think about that possibility. I checked the code and it just prints something like

"": { ... }

which I doubt will be very useful. ~~I decided to require that the input string be non-empty.~~

Initially, I felt that it might be an over-kill, but the original issue was that we have the word value0, so replacing it with the empty string will not really be much of an improvement, so I figured that I'd require it. Let me know what you think!

I think this is ok, but you might want to do a ReleaseAssert / ReleaseAssertOrThrow (to ensure the assert remains inside release builds) and of course make sure tests all pass. maybe also run them at trace log-level in case we missed any cases.

@graydon
I didn't know a bout ReleaseAssert! Although after some thinking, I started to wonder that if it'd be better if the assert does NOT remain inside release builds. I'm currently thinking that this issue of printing "": { may not be important enough to warrant an abort or throw in production.

I just confirmed that all the tests do pass.

Let me know what you think!

After more thoughts & learning about #2852, I started thinking that it may be better to not have any assertion statements. Printing "": { doesn't seem bad enough to warrant an abort or throw in production.

jonjove · 2021-02-22T21:46:02Z

src/util/XDRCereal.h

 {
-    std::vector<std::string> tmp;
-    for (auto const& h : s)
+    // setNextName, startNode, and finishNode create a new sub-object


You might add some details about the fact that setNextName, startNode, ..., finishNode is basically what ar(cereal::make_nvp(....)) does.

Updated the comment. As I was updating it, I started to think that explicitly calling prologue and epilogue might be easier for future developers to understand since it'll be more similar to ar(), so I went ahead and replaced them. Let me know what you think!

src/util/XDRCereal.h

graydon

I agree with the current structure for printing containers. Conversation with @jonjove the other day made sense, and this code matches my current recollection of that conversation.

… would print "value0"

graydon · 2021-03-04T00:28:09Z

r+ 8df71d8

marta-lokhova reviewed Jan 13, 2021

View reviewed changes

hidenori-shinohara force-pushed the xdr-value0 branch from fd1b300 to 67583aa Compare January 13, 2021 23:18

jonjove mentioned this pull request Jan 15, 2021

Fix cereal for containers xdrpp/xdrpp#25

Open

jonjove reviewed Jan 15, 2021

View reviewed changes

MonsieurNicolas added this to In progress in v15.3.0 via automation Jan 16, 2021

hidenori-shinohara force-pushed the xdr-value0 branch from b2420cd to 746b539 Compare February 4, 2021 19:10

hidenori-shinohara requested review from jonjove and marta-lokhova February 4, 2021 19:15

hidenori-shinohara force-pushed the xdr-value0 branch from 746b539 to 59cf329 Compare February 11, 2021 16:22

jonjove reviewed Feb 11, 2021

View reviewed changes

hidenori-shinohara force-pushed the xdr-value0 branch 3 times, most recently from c9e2697 to a4b4e35 Compare February 12, 2021 17:39

hidenori-shinohara requested a review from jonjove February 12, 2021 17:44

MonsieurNicolas added this to In progress in v15.4.0 via automation Feb 15, 2021

MonsieurNicolas removed this from In progress in v15.3.0 Feb 15, 2021

hidenori-shinohara force-pushed the xdr-value0 branch 2 times, most recently from b048c86 to 58b21bb Compare February 22, 2021 21:30

jonjove reviewed Feb 22, 2021

View reviewed changes

hidenori-shinohara force-pushed the xdr-value0 branch 2 times, most recently from a3c309e to 66d3145 Compare February 23, 2021 19:19

graydon reviewed Feb 23, 2021

View reviewed changes

graydon approved these changes Feb 23, 2021

View reviewed changes

hidenori-shinohara force-pushed the xdr-value0 branch from 66d3145 to 81b600a Compare February 25, 2021 16:15

hidenori-shinohara force-pushed the xdr-value0 branch from 81b600a to fae18e1 Compare February 25, 2021 21:34

Hidenori Shinohara added 4 commits February 25, 2021 16:34

Add issuer information when printing Asset

f44d2a3

Update the way a container is printed

aa7f719

Make the name field of xdr_to_string mandatory since otherwise Cereal…

d7d7283

… would print "value0"

Make xdr_to_string use xdr::archive

8df71d8

hidenori-shinohara force-pushed the xdr-value0 branch from fae18e1 to 8df71d8 Compare February 25, 2021 21:34

latobarita merged commit edba91c into stellar:master Mar 4, 2021

v15.4.0 automation moved this from In progress to Done Mar 4, 2021

		@@ -196,8 +198,10 @@ checkResults(Application& app, uint32_t ledger,
		"Expected result code {} does not agree with {} for "

Fix the rendering issues of some XDR fields #2876

Fix the rendering issues of some XDR fields #2876

Conversation

hidenori-shinohara commented Jan 13, 2021

Description

Checklist

jonjove commented Jan 13, 2021

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

hidenori-shinohara commented Jan 13, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidenori-shinohara commented Feb 11, 2021

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

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

hidenori-shinohara commented Feb 22, 2021

jonjove left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidenori-shinohara Feb 23, 2021 • edited

Choose a reason for hiding this comment

graydon Feb 23, 2021 • edited

Choose a reason for hiding this comment

hidenori-shinohara Feb 25, 2021 • edited

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

graydon left a comment

Choose a reason for hiding this comment

graydon commented Mar 4, 2021

hidenori-shinohara commented Jan 13, 2021 •

edited

hidenori-shinohara Feb 23, 2021 •

edited

graydon Feb 23, 2021 •

edited

hidenori-shinohara Feb 25, 2021 •

edited