Add ORDER_PAYMENT network message #1654
Conversation
…function to populate the txnOutputs for use in the order payment message handler
drwasho
added
the
🔍 readyForReview
drwasho
added
🚧 in progress
| if err != nil { | ||
| return txn, err | ||
| } | ||
| if len(addrs) == 0 { |
There was a problem hiding this comment.
This code in each of the wallet implementations has a small chance of putting us in an unrecoverable state. For example, suppose sender of this transaction included a change output that was not of a standard address type. The change output is for their wallet, not ours, so technically this would still be a valid transaction on the network. However, our code would error here. Either where err != nil or where len(addrs) == 0.
So I think it's probably best to just set the address to "" in this case rather than error. Maybe log a warning as well.
drwasho
added
🔍 readyForReview
|
I also don't see any godep updates at all. @cpacia if we're going to completely ignore that file, we should probably just remove it? I don't godep has any more use to us at this point. But that leaves the concern that we aren't managing the vendors folder properly which is greatly concerning. I don't know how we are ensuring that these commits are being included in the dependent libs. |
|
Final TODO @amangale can you send a PR to to the upstream repos with the changes that are being included in vendor? Thanks |
There was a problem hiding this comment.
This is looking ready to merge. I've looked at the upstream dependencies and I'm okay if we fix those later. There are a few error cases which should be remote edge scenarios but at least the logging looks good enough to give us enough info to understand how to better protect it if needed. If @cpacia can approve his change requests as well, we can merge this and resolve remaining nits in a following PR.
|
Great work, @amangale, keeping up with the critiques. 🍻 Thanks! |
placer14
force-pushed
the
order_payment_network_message
branch
from
586b1c0 to
75eed3e
Compare
placer14
changed the title
placer14
removed
the
🔍 readyForReview
This PR extracts the ORDER_PAYMENT network message code from the existing ethereum branch to be merged independently.