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

Minor asset loop out fixes #876

Merged
merged 4 commits into from
Jan 22, 2025

Conversation

sputn1ck
Copy link
Member

@sputn1ck sputn1ck commented Jan 22, 2025

This PR adds some minor asset loop out fixes:

  • Fix max limit for rfq
  • Clarify max limit naming
  • Add asset rate display to quote

This is what a loop out quote will look like now:
image

Copy link
Collaborator

@hieblmi hieblmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I think a unit test around the fixpoint conversion would make sense.

assets/client.go Outdated Show resolved Hide resolved
client.go Outdated Show resolved Hide resolved
cmd/loop/quote.go Outdated Show resolved Hide resolved
looprpc/client.proto Outdated Show resolved Hide resolved
looprpc/client.proto Show resolved Hide resolved
cmd/loop/quote.go Show resolved Hide resolved
@sputn1ck sputn1ck force-pushed the asset_loop_out_fixes branch from 484d049 to 32647a5 Compare January 22, 2025 11:49
@sputn1ck sputn1ck requested a review from hieblmi January 22, 2025 11:49
Copy link
Collaborator

@starius starius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🏆
Added few comments.

assets/client.go Outdated Show resolved Hide resolved
interface.go Outdated Show resolved Hide resolved
looprpc/client.proto Show resolved Hide resolved
//
// * `F_s` is the scale component. It is an integer specifying how
// many decimal places `F_c` should be divided by to obtain the fractional
// representation.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type is the same as github.com/lightninglabs/taproot-assets/taprpc/rfqrpc.FixedPoint
Can we include .proto file from taproot-assets?
If this is not possible, I propose to add TODO to deduplicate this in the future.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


// unmarshalFixedPoint converts an RPC FixedPoint to a BigIntFixedPoint.
func unmarshalFixedPoint(fp *looprpc.FixedPoint) (*rfqmath.BigIntFixedPoint,
error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose to reuse https://pkg.go.dev/github.com/lightninglabs/taproot-assets/taprpc/rfqrpc#UnmarshalFixedPoint
looprpc.FixedPoint is the same type as rfqrpc.FixedPoint (has the same fields).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not seem to work for proto messages,

image

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible to create a struct manually filling the fields.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

created a rfqrpc.FixedPoint struct and called rfqrpc.UnmarshalFixedPoint

Comment on lines 813 to 816
SwapAssetRate: &looprpc.FixedPoint{
Scale: uint32(quote.LoopOutRfq.SwapAssetRate.Scale),
Coefficient: quote.LoopOutRfq.SwapAssetRate.Coefficient.String(),
},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose to factor out this transformation to a function, e.g. marshalFixedPoint.
It is used twice, for PrepayAssetRate and for SwapAssetRate.

SwapAssetRate: marshalFixedPoint(quote.LoopOutRfq.SwapAssetRate),

We can also cover functions marshalFixedPoint and unmarshalFixedPoint with a unit test making sure everything is converted properly back and forth.

@sputn1ck sputn1ck force-pushed the asset_loop_out_fixes branch from 32647a5 to 9008b3b Compare January 22, 2025 14:33
@sputn1ck sputn1ck force-pushed the asset_loop_out_fixes branch from 9008b3b to d0191d2 Compare January 22, 2025 14:41
@sputn1ck sputn1ck merged commit bb859c5 into lightninglabs:master Jan 22, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants