rfqmsg: detect asset related custom records correctly #1322
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pull Request Test Coverage Report for Build 12988277780Details
💛 - Coveralls |
gijswijs
approved these changes
Jan 24, 2025
| rfqIDType HtlcRfqIDType | ||
| ) | ||
| for key := range records { | ||
| if key == uint64(amountType.TypeVal()) { |
There was a problem hiding this comment.
This returns true also if there's only one of the two custom records. Are there cases where we expect one but not the other?
There was a problem hiding this comment.
That's the asset keysend case I believe, there's no RFQ involved in that flow
There was a problem hiding this comment.
Correct. Technically an HTLC isn't valid if it has only the RFQ ID an no amount (but just amount and no RFQ ID is correct for the keysend case as George mentioned).
But that logic is out of scope for this function IMO, we just want to know if there are any asset related records present.
GeorgeTsagk
approved these changes
Jan 24, 2025
| rfqIDType HtlcRfqIDType | ||
| ) | ||
| for key := range records { | ||
| if key == uint64(amountType.TypeVal()) { |
There was a problem hiding this comment.
That's the asset keysend case I believe, there's no RFQ involved in that flow
|
kicking the itests |
To save some space and not have a false positive on custom record detection, we only encode the HTLC asset amounts if the list is actually non-empty. This is mostly to get the unit test in the next commit to work and shouldn't have any direct real-world impact (as we should never encode the HTLC struct into an actual wire message if there aren't any values set inside the struct).
To detect whether an HTLC's custom records fields are values as we'd expect them to be set for an asset HTLC, we add a new helper function.
guggero
force-pushed
the
conditional-records
branch
from
5317009 to
c97b302
Compare
To not interfere with other custom records, we only want to count an HTLC as an asset HTLC if it actually contains asset related custom records.
guggero
force-pushed
the
conditional-records
branch
from
c97b302 to
d2271e0
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1321.