Skip to content
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

Merged
merged 3 commits into from
Nov 21, 2024

Conversation

guggero
Copy link
Member

@guggero guggero commented Nov 14, 2024

Fixes #1053.

Adds basic validation to AddAssetBuyOrder and AddAssetSellOrder 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.

@guggero guggero requested review from ffranr and jharveyb November 14, 2024 17:11
@coveralls
Copy link

coveralls commented Nov 14, 2024

Pull Request Test Coverage Report for Build 11955125756

Details

  • 0 of 59 (0.0%) changed or added relevant lines in 1 file are covered.
  • 10 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.006%) to 41.23%

Changes Missing Coverage Covered Lines Changed/Added Lines %
rpcserver.go 0 59 0.0%
Files with Coverage Reduction New Missed Lines %
tappsbt/create.go 2 53.22%
commitment/tap.go 2 84.17%
tapchannel/aux_leaf_signer.go 3 36.33%
tapgarden/caretaker.go 3 68.59%
Totals Coverage Status
Change from base Build 11953454712: -0.006%
Covered Lines: 25552
Relevant Lines: 61975

💛 - Coveralls

Copy link
Contributor

@ffranr ffranr left a 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?).

rfq/manager.go Outdated Show resolved Hide resolved
rfq/negotiator.go Outdated Show resolved Hide resolved
taprpc/rfqrpc/rfq.proto Outdated Show resolved Hide resolved
rpcserver.go Outdated Show resolved Hide resolved
rpcserver.go Outdated Show resolved Hide resolved
@guggero guggero self-assigned this Nov 19, 2024
@guggero guggero force-pushed the rfq-order-validation branch from e6f0f42 to 7b19ac1 Compare November 19, 2024 19:26
@guggero guggero requested a review from ffranr November 19, 2024 19:26
taprpc/rfqrpc/rfq.proto Outdated Show resolved Hide resolved
@guggero guggero force-pushed the rfq-order-validation branch 3 times, most recently from 4dd7b40 to e9d2fb0 Compare November 19, 2024 20:08
@guggero
Copy link
Member Author

guggero commented Nov 19, 2024

@ffranr sorry for the premature request for review before. Everything should be green now (minus flakes) and comments addressed.

@guggero guggero requested a review from ffranr November 19, 2024 20:20
@guggero guggero force-pushed the rfq-order-validation branch from e9d2fb0 to ce863b1 Compare November 20, 2024 15:06
Copy link
Contributor

@gijswijs gijswijs left a 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.

rpcserver.go Outdated Show resolved Hide resolved
@guggero guggero force-pushed the rfq-order-validation branch from ce863b1 to e0d9ea1 Compare November 21, 2024 14:19
@guggero guggero merged commit 39d1889 into main Nov 21, 2024
17 of 18 checks passed
@guggero guggero deleted the rfq-order-validation branch November 21, 2024 14:57
@dstadulis dstadulis added this to the v0.5 (v0.4.2 rename) milestone Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

[feature]: add channel conditional validation to AddAsset*Order RPC endpoints
5 participants