Preprocess _muxed_id fields before unmarshalling from the DB #3722
Conversation
|
upon checking all the I think we are fine with |
| @@ -24,15 +25,39 @@ func (r *Operation) UnmarshalDetails(dest interface{}) error { | |||
| if !r.DetailsString.Valid { | |||
| return nil | |||
| } | |||
|
|
|||
| err := json.Unmarshal([]byte(r.DetailsString.String), &dest) | |||
| preprocessedDetails, err := preprocessDetails(r.DetailsString.String) | |||
There was a problem hiding this comment.
Shouldn't we do the same thing for effects? I'll update an integration test to generate a trade.
There was a problem hiding this comment.
OK, it is working because we are lucky:
account_muxed_idis done inPopulateBaseEffectso it's not unmarshaling directly to other struct.seller_muxed_idis never populated during ingestion, even when seller is a muxed account. This is likely ingestion bug or we just can't extract this information during ingestion.
There was a problem hiding this comment.
* `seller_muxed_id` is never populated during ingestion, even when seller is a muxed account. This is likely ingestion bug or we just can't extract this information during ingestion.
I think it is populated (see addAccountAndMuxedAccountDetails(sd, buyer, "seller") in tradeDetails()).
The reason I think it works is because we use an intermediate datastructure (history.TradeEffectDetails, which expects an uint64) before copying the fields to the final effects.Trade data structure (which does have the ,string json tag) in when unmarshalling the details from the database in NewEffect().
This BTW confirms that we don't need a ,string tag in services/horizon/internal/db2/history/main.go (which I wanted to check in #3722 (comment) ).
There was a problem hiding this comment.
@2opremio you are correct. I missed it when checking my test results because I forgot there are always two trade effects for each trade.
| func (am *AccountMerge) Validate(withMuxedAccounts bool) error { | ||
| var err error | ||
| if withMuxedAccounts { | ||
| _, err = xdr.AddressToAccountId(am.Destination) | ||
| } else { | ||
| _, err = xdr.AddressToMuxedAccount(am.Destination) | ||
| } else { | ||
| _, err = xdr.AddressToAccountId(am.Destination) | ||
| } | ||
| if err != nil { | ||
| return NewValidationError("Destination", err.Error()) |
There was a problem hiding this comment.
I changed the order here to be able to merge account into muxed destination. This should be probably a part of txnbuild patch release. @stellar/horizon-committers
Fixes #3714 . In particular it fixed the problem found at #3716 (comment)