-
Notifications
You must be signed in to change notification settings - Fork 132
misc: fix litcli command docs, add group key support to DecodeAssetPayReq #1498
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
base: main
Are you sure you want to change the base?
Conversation
While we are updating docs, can we add a comment |
Yes, added that here: lightninglabs/lightning-terminal#1052. |
Not seeing that. Here is what I get
but I do here, which is a mistake
|
Oops, yeah, added it to the wrong command. Updated in lightninglabs/lightning-terminal#1052 (can you please comment CLI related issues there so we don't mix responsibilities in the PRs?). |
When running |
Makes sense. I've added a condition that only prints that information if we requested to decode the invoice with an asset ID and not a group key. |
Pull Request Test Coverage Report for Build 14835139228Details
💛 - Coveralls |
@@ -264,6 +265,25 @@ func (b *Book) QueryAssetInfo(ctx context.Context, | |||
return assetGroup, nil | |||
} | |||
|
|||
// SyncAssetGroup attempts to enable asset sync for the given asset group, then | |||
// perform an initial sync with the federation for that group. | |||
func (b *Book) SyncAssetGroup(ctx context.Context, |
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.
Alternatively, we can have QueryAssetInfo
accept an asset.Specifier
.
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.
The way the function is setup, if you call it twice, only the first time will it actually try to fetch data.
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.
But QueryAssetInfo
does the opposite of what we want: Given an asset ID it finds out what the group key is.
But what we need for this use case is: Given a group key, what asset IDs does that group contain?
leafKeys, err := r.cfg.Multiverse.UniverseLeafKeys( | ||
ctx, universe.UniverseLeafKeysQuery{ | ||
Id: leafID.ID, | ||
Limit: 1, |
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.
What order are we actually relying on here?
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.
No specific order (meaning natural database order), since it's not really important which asset we get.
tapdLog.Tracef("Found %d leaves for group key %x", | ||
len(leaves), groupKey.SerializeCompressed()) | ||
|
||
// If there are no leaves, then we need to sync the asset group. |
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'm thinking you should sync regardless in order to make sure you use the asset_id
of the first minted traunch is used so that decimal_display
is enforced properly. See also, #1503 .
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.
what did you think about this comment?
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.
We'll enforce that all assets minted will have the same decimal display. But that will be a different PR that fixes #1503. At this point we need to be able to assume that any asset of the same group key has the same properties in this regard. Finding out which one was the actual first tranche isn't easy to do here.
@guggero do you see my review comments for this one too now? |
We'll want to be able to query the asset leaves for a group key to map them to one or more asset IDs.
Since we support group keys in all other payment related commands, we now add the same argument for the invoice decoding RPC.
Fixes a couple of smaller issues.
Fixes #1496.
Fixes #1493.
PR with itests is here: lightninglabs/lightning-terminal#1052