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

mintrpc+tapgarden: add universe announcement flag to asset minting #1173

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ffranr
Copy link
Contributor

@ffranr ffranr commented Nov 5, 2024

This PR adds a new flag to the MintAsset RPC request message. When set to true, this flag enables the universe announcement feature for the minting batch. To comply with this feature, the minting batch must be restricted to assets that share the same group key.


This PR is part of the work towards closing: #1094

Move validation logic for MintAsset RPC requests into a standalone
function to improve readability. This makes it clearer where the bulk
of the request validation logic ends.
Remove inaccurate statement in `prepAssetSeedling` doc comment.

Improve clarity of comment at `prepAssetSeedling` call site.
Update doc comment for PendingBatch to improve clarity. Test to verify
that nil is returned when no pending batch exists.
This commit adds a new flag to the `MintAsset` RPC request message. When
set to true, this flag enables the universe announcement feature for the
minting batch. To comply with this feature, the minting batch must be
restricted to assets that share the same group key.
@ffranr ffranr requested a review from jharveyb November 5, 2024 15:05
@ffranr ffranr self-assigned this Nov 5, 2024
@coveralls
Copy link

Pull Request Test Coverage Report for Build 11686896618

Details

  • 25 of 91 (27.47%) changed or added relevant lines in 3 files are covered.
  • 30 unchanged lines in 6 files lost coverage.
  • Overall coverage increased (+0.003%) to 40.685%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tapgarden/planter.go 19 22 86.36%
tapgarden/batch.go 6 20 30.0%
rpcserver.go 0 49 0.0%
Files with Coverage Reduction New Missed Lines %
commitment/tap.go 1 83.64%
tapdb/addrs.go 2 79.04%
tappsbt/create.go 2 53.22%
asset/asset.go 2 80.02%
tapgarden/caretaker.go 4 68.5%
universe/interface.go 19 47.09%
Totals Coverage Status
Change from base Build 11684840814: 0.003%
Covered Lines: 24709
Relevant Lines: 60733

💛 - Coveralls

Copy link
Contributor

@jharveyb jharveyb left a comment

Choose a reason for hiding this comment

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

Looks good overall! Still some bits to figure out about exactly which constraints will apply to the batches here.

@@ -574,7 +597,7 @@ func (r *rpcServer) MintAsset(ctx context.Context,
}
}

if specificGroupInternalKey {
if req.Asset.GroupInternalKey != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't have the same meaning as before. specificGroupInternalKey was true if the seedling is being minted into an existing group.

GroupInternalKey is only non-nil if you are creating a new group, and you want to use a specific internal key instead of whichever key tapd will generate automatically.

@@ -309,6 +315,35 @@ func (m *MintingBatch) HasSeedlings() bool {
return len(m.Seedlings) != 0
}

// ValidateSeedling checks if a seedling is valid for the batch.
func (m *MintingBatch) ValidateSeedling(newSeedling Seedling) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC we want this to be a logical AND between this flag for each seedling, and the matching flag for the batch.

So the first seedling of the batch should update the batch flag, and every other seedling should match. This implies that every seedling would have EnableUniAnnounce set to true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's what I'm going for. EnableUniAnnounce set to true on each seedling if it's true on the batch. They need to correspond basically. We can relax that sort of thing in the future. Let me know if you have thoughts on that sort of approach.

tapgarden/batch.go Show resolved Hide resolved
tapgarden/planter.go Show resolved Hide resolved
tapgarden/planter_test.go Show resolved Hide resolved
taprpc/mintrpc/mint.proto Show resolved Hide resolved
@Roasbeef Roasbeef added the P1 label Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

5 participants