-
Notifications
You must be signed in to change notification settings - Fork 116
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[rfq]: add validation to AddAssetBuyOrder
and AddAssetSellOrder
RPCs
#1192
Conversation
Pull Request Test Coverage Report for Build 11955125756Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should consider dropping the term "balance" from the new req arg name.
We can just call it SkipAssetChannelCheck
and the "channel check" can include checking for an existing channel and any balance check that we may add later (is absent from this PR i think?).
e6f0f42
to
7b19ac1
Compare
4dd7b40
to
e9d2fb0
Compare
@ffranr sorry for the premature request for review before. Everything should be green now (minus flakes) and comments addressed. |
e9d2fb0
to
ce863b1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK 🎉
Just one nit.
I would like to see a future where tapd
can be tested with some sort of a lnd
-mock so that we can mimick proper asset channels, and not need to depend on flags like this.
ce863b1
to
e0d9ea1
Compare
Fixes #1053.
Adds basic validation to
AddAssetBuyOrder
andAddAssetSellOrder
RPCs: We want to have a channel with the given asset to the given peer when adding an order. Otherwise RFQ negotiation doesn't make sense.We add an exception for our integration tests, where we can't create full-blown asset channels (in the
tapd
standalone integration tests). A new flag can be specified that then loosens the requirement to just a normal, active channel with the peer.