-
Couldn't load subscription status.
- Fork 138
Burn assets by group key #1812
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
Burn assets by group key #1812
Conversation
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.
Great work!
13a399f to
9cd15a7
Compare
9cd15a7 to
7c6e2ba
Compare
Pull Request Test Coverage Report for Build 18141060997Details
💛 - Coveralls |
a9fc25c to
544f5b4
Compare
c0a0cae to
c3082d6
Compare
86b225f to
ca49ac9
Compare
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.
getting there
some final Qs
24569f4 to
34ff428
Compare
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.
LGTM, pending some nits
|
We'll have to rebase |
965998d to
83a4116
Compare
34ff428 to
35da11c
Compare
|
Oops @GeorgeTsagk, deleted a comment but wasn't on purpose. Here is the response:
|
Allow the creation of multiple vPackets when burning by group key.
35da11c to
926db4e
Compare
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.
re-ACKing
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.
Thank you for your continued effort and patience with this PR. I just want to confirm that we're aligned on the behavior for asset-ID-specific burning of grouped assets.
| // Handle different scenarios based on what was provided: | ||
| // 1. If only assetID is provided, look up the group key. | ||
| // 2. If only groupKey is provided, use it directly | ||
| // 3. If both are provided, validate consistency. | ||
| switch { | ||
| case assetID != nil && groupKey == nil: | ||
| assetGroup, err := r.cfg.TapAddrBook.QueryAssetGroupByID( | ||
| ctx, *assetID, | ||
| ) | ||
| switch { | ||
| case err == nil && assetGroup.GroupKey != nil: | ||
| groupKey = &assetGroup.GroupPubKey | ||
|
|
||
| case errors.Is(err, address.ErrAssetGroupUnknown): | ||
| rpcsLog.Trace("Asset group key not found, asset " + | ||
| "may not be part of a group") | ||
|
|
||
| case err != nil: | ||
| return nil, fmt.Errorf("error querying asset "+ | ||
| "group: %w", err) | ||
| } |
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 thought the intended behaviour was:
- If only an asset ID is specified, burn assets matching that asset ID.
- If only an asset group is specified, burn across any tranche within that group.
- If both an asset ID and an asset group are specified, burn assets matching the asset ID, but verify that the asset belongs to the specified group.
This approach allows burning assets across a group when the caller doesn’t care about the tranche, while preserving the option to target specific tranches (asset IDs) when needed.
Perhaps something like this:
https://gist.github.com/ffranr/9b94850c4fc828a1a3e5170b1d1af199
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.
// 1. If only assetID is provided, look up the group key.
This part concerns me slightly. Do we assert in the integration test that when a group collectable is burned by asset ID, only that specific asset ID is burned and not another collectable from the same group? Why do we need to populate the group key when only an asset ID is specified?
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.
- If only an asset ID is specified, burn assets matching that asset ID.
- If only an asset group is specified, burn across any tranche within that group.
- If both an asset ID and an asset group are specified, burn assets matching the asset ID, but verify that the asset belongs to the specified group.
This is what the code does. Did you find a bug in the current logic that somehow doesn't execute this flow?
Why do we need to populate the group key when only an asset ID is specified
This branche of the PR is the one that does not change. As you can see, the previous code already populates the group key when only the assetID is specified. My goal was specifically not to alter the previous behavior. I can investigate the reason why the group key is needed but that should not matter for this PR unless we want to change the previous logic.
Return multiple proofs in BurnAssetResponse.
926db4e to
5b69361
Compare
Enable burning of assets by specifying a group key.
AssetSpecifiertotapcommon.protoand use it in theBurnAssetRequesttapclito acceptgroup_keyparam intapd assets burncommandstapfreighterto support passing a group key in the asset specifierBurnAssetRPC request to accept anAssetSpecifier. Breaking change!Example: