minting: fix re-issuing into existing group with external key

[guggero]

Fixes #1515.
Fixes #1516.

[ZZiigguurraatt]

so this is ready to test?

[guggero]

so this is ready to test?

Yes.

[coveralls]

Pull Request Test Coverage Report for Build 14902741297

Details

• 6 of 47 (12.77%) changed or added relevant lines in 4 files are covered.
• 17 unchanged lines in 8 files lost coverage.
• Overall coverage decreased (-0.01%) to 36.918%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
tapgarden/planter.go	6	8	75.0%
tapgarden/batch.go	0	4	0.0%
rpcserver.go	0	11	0.0%
tapgarden/seedling.go	0	24	0.0%

Files with Coverage Reduction	New Missed Lines	%
commitment/tap.go	1	72.05%
tapgarden/batch.go	1	71.88%
asset/asset.go	2	49.17%
asset/mock.go	2	65.45%
internal/test/helpers.go	2	86.95%
tappsbt/create.go	2	26.74%
rpcserver.go	3	0.0%
tapgarden/caretaker.go	4	68.68%

Totals
Change from base Build 14895618326:	-0.01%
Covered Lines:	26530
Relevant Lines:	71862

---

💛 - Coveralls

[ZZiigguurraatt]

are you able to put these commits on top of #1498 as that is what I'm currently testing with ?

is that going to work okay as long as you merge #1498 first?

[ZZiigguurraatt]

I'm able to get this PR to work.

One additional thing I've now tested is the group_anchor option. Does that only work to mint two tranches in the same batch (without having a group_key first)? I'm trying to use it in a second batch, but I get

unable to mint asset: batch empty, group anchor Asset1 invalid

If it is only for minting two tranches in the same batch, why do we need to do that? Seems to me like if someone is going to be minting the same asset in the same batch, they would never need two tranches? I'm just wondering if this option should be removed if that is its only purpose. If it should not be removed, I think we should change the description from

The name of the asset in the batch that will anchor a new asset group.
This asset will be minted with the same group key as the anchor asset.

to

The name of a new asset in the same batch that will anchor a new asset group.
This asset will be minted with the same group key as the anchor asset.
This option cannot be used to anchor to an asset that has already been finalized in a previous batch.

If it is supposed to work from a second batch, I think we should fix it.

[guggero]

Does that only work to mint two tranches in the same batch (without having a group_key first)?

Yes, the group anchor (string) argument is only for tranches in the same batch.
I know we need to improve the documentation in this area. But basically the flags/fields work this way:

• new_grouped_asset: I want to create a new group and this asset starts the group.
• grouped_asset: This asset belongs to a group and I'll specify what group that is with other params (can either be group_anchororgroup_key`)
• group_internal_key: When starting a new group, instruct lnd to use this specific key from the keychain to sign for the group (only works with new_grouped_asset).
• external_group_key: Don't use a group internal key that is owned by lnd, use the externally owned key specified in this struct. This is required for both starting a group as well as re-issuing into an existing group that uses the external group (these are basically signing instructions).
• group_anchor: Another asset in the current batch started the group, this asset should be put in the same group as the one in the batch with the name given (use case: multiple collectibles that belong to the same group but are minted in the same transaction). Reason for this being a string: In the case where the user doesn't want to define what group key they want to assign, one is created automatically. But this is done when sealing the batch, so we can't reference it as an actual public key here. That's why we specify the group anchor by its name (string).
• group_key: Another asset in another batch/transaction started the group. This asset should join the group that has this group public key.

Feel free to use this in any way to update the docs...

[ZZiigguurraatt]

Does that only work to mint two tranches in the same batch (without having a group_key first)?

Yes, the group anchor (string) argument is only for tranches in the same batch. I know we need to improve the documentation in this area. But basically the flags/fields work this way:

* `new_grouped_asset`: I want to create a new group and this asset starts the group.

* grouped_asset`: This asset belongs to a group and I'll specify what group that is with other params (can either be `group_anchor`or`group_key`)

* `group_internal_key`: When starting a new group, instruct `lnd` to use this specific key from the keychain to sign for the group (only works with `new_grouped_asset`).

* `external_group_key`: Don't use a group internal key that is owned by `lnd`, use the externally owned key specified in this struct. This is required for both starting a group as well as re-issuing into an existing group that uses the external group (these are basically signing instructions).

* `group_anchor`: Another asset in the current batch started the group, _this_ asset should be put in the same group as the one in the batch with the name given (use case: multiple collectibles that belong to the same group but are minted in the same transaction). Reason for this being a string: In the case where the user doesn't want to define what group key they want to assign, one is created automatically. But this is done when sealing the batch, so we can't reference it as an actual public key here. That's why we specify the group anchor by its name (string).

* `group_key`: Another asset in another batch/transaction started the group. This asset should join the group that has this group public key.

Feel free to use this in any way to update the docs...

This helps a lot. Linking to this comment in #1519 so that we can deal with it there.

[ffranr]

Instead of this switch you can just return early with:

if c.ExternalKey.IsNone() && !group.GroupKey.IsLocal() {
	groupPubKey := c.GroupInfo.GroupPubKey
	groupKeyBytes := groupPubKey.SerializeCompressed()
	return fmt.Errorf("can't sign with group key %x",
		groupKeyBytes)
}

And you don't need to wrap the fn.MapOptionZ call in case c.ExternalKey.IsSome():. Because fn.MapOptionZ will just return err == nil if c.ExternalKey.IsNone(). Maybe we can save on indentation like that.

[guggero]

Ah nice, that's actually much cleaner. Updated, thanks.

[ffranr]

Why is mintReq2.Asset.GroupedAsset = true necessary here? I would have expected setting mintReq2.Asset.ExternalGroupKey to imply group asset sufficiently.

Is setting these fields necessary or are we just covering all possibilities here?

[guggero]

The RPC (and by extension the CLI) interface for the minting call is not optimal and quite confusing. See #1517 (comment)

So you need to set at least one of the GroupedAsset or NewGroupedAsset to true if you want to start or issue into a group, independent of the other fields.

[ffranr]

thanks for the fix

[GeorgeTsagk]

Looks good! 🌴

[ZZiigguurraatt]

I have been having good results with this one. Thanks for the quick fix and so much feedback. Glad to see it merged!