Invalid ORDER_PAYMENT generated

[placer14]

Was testing the dispute payout flow and found the new ORDER_PAYMENT message had an invalid transaction ID. Here is an example of the protobuf data I encountered:

*github.com/OpenBazaar/openbazaar-go/pb.OrderPaymentTxn {
        Coin: "LTC",
        OrderID: "QmYpRLooYFK6vndoVwaJdpV5pPz2tyyuPCRjqxVpeub4qE",
        TransactionID: "01000000000101d34ccc6db56e98f446195090d56592bfb3895f41d36e39241f...+706 more",
        WithInput: true,
        XXX_NoUnkeyedLiteral: struct {} {},
        XXX_unrecognized: []uint8 len: 0, cap: 0, nil,
        XXX_sizecache: 0,}

The steps to reproduce this: 1) Create a moderated order and open a dispute. 2) Moderator resolves 100% of funds to the buyer. 3) Buyer accepts payout, generating the invalid ORDER_PAYMENT message to send to the vendor. 4) Observe the bad message data named paymentDetails within net/service/handlers.go.handleOrderPayment within this block of code:

1835   paymentDetails := new(pb.OrderPaymentTxn)
1836   err := ptypes.UnmarshalAny(pmes.Payload, paymentDetails)
1837   if err != nil {
1838     return nil, err
1839   }

This does not seem to prevent funds from resolving in this case, as the resolution was sent 100% to the buyer. Unsure what would happen with split payment resolution.

[placer14]

The source of this bug is due to our non-ETH wallet's Multisign return value returning the tx script, which is inconsistent with the ETH wallet (which only returns the tx hash). This value is being used inside of OrderPayment messages on OpenBazaarNode.ReleasePayment() and causing the vendor node which receives it to fail to process it properly. This is probably inconsequential because only ETH requires the message at all, but we have not designed this logic to be optional and should be consistent with how it's being consumed for all wallet cases.

Decided that we would make our non-ETH wallet's return value be a txid (taking care to ensure BigEndian txids via hex.Decode((wire.MsgTx).TxHash().String()). Changes will be made in-place within the vendors folder and we will reconcile dependencies when eth-master is merged.

[placer14]

Due to the inconsequential nature of this bug, we will postpone it's resolution until after more critical items are resolved.

[hoffmabc]

@drwasho since this is not a show stopper should we defer this?

[drwasho]

Yes I think so.