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.

Sign up for GitHub

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

Jump to bottom

split enableEmission into newGroupedAsset and groupedAsset #681

Merged
merged 5 commits into from
Nov 21, 2023
Merged

split enableEmission into newGroupedAsset and groupedAsset #681

merged 5 commits into from
Nov 21, 2023

Conversation

jharveyb
Copy link
Contributor

Fixes #236 , #627 .

We replace the enableEmission flag with two flags, newGroupedAsset and groupedAsset, at both the RPC and CLI level.

Correct usage:

Asset that creates an asset group, allows future emission / reissuance: --new_grouped_asset

Asset minted into an asset group specified by group key: --grouped_asset --group_key

Asset minted into the same group as another asset in the same minting batch, specified by asset name: --grouped_asset --group_anchor

We also fix a small bug where displaying a pending batch would not show if an asset had --new_grouped_asset set, and port a fix for a logging issue that could cause a crash.

@jharveyb jharveyb self-assigned this Nov 16, 2023
@jharveyb jharveyb force-pushed the enable_emission_flag_split branch from 3696293 to 1f43cf5 Compare November 16, 2023 21:27
@dstadulis dstadulis added this to the v0.3.2 milestone Nov 20, 2023
@dstadulis dstadulis added the cli label Nov 20, 2023
GeorgeTsagk
GeorgeTsagk reviewed Nov 20, 2023
View reviewed changes
Copy link
Member

@GeorgeTsagk GeorgeTsagk left a comment •
edited
Loading

Choose a reason for hiding this comment

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

PR looks good, will also do some manual testing

vectors seem to be failing? (kicked it, will see if it passes)

ffranr
ffranr approved these changes Nov 20, 2023
View reviewed changes
Copy link
Contributor

@ffranr ffranr left a comment

Choose a reason for hiding this comment

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

Great to see this cleared up 👍

taprpc/mintrpc/mint.proto Outdated
@@ -33,40 +33,74 @@ service Mint {
rpc ListBatches (ListBatchRequest) returns (ListBatchResponse);
}

message MintAsset {
message MintSeedling {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the first time we're exposing users to the concept of a "seedling"? Perhaps we should add a doc comment to explain what this message type is.

I don't think we should hold up this PR on terminology. But perhaps we should think carefully about "seepling" if this is the first time a user might encounter that word in this context. MintCandidateAsset?

Copy link
Member

Choose a reason for hiding this comment

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

IIUC, this will also trigger a larger breaking change as well (proto compilation failing). Seems non-essential?

Copy link
Member

Choose a reason for hiding this comment

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

Looks like just for display, could be PendingBatchAsset, or just PendingAsset.

Roasbeef
Roasbeef reviewed Nov 20, 2023
View reviewed changes
tapgarden/caretaker.go
@@ -785,7 +785,7 @@ func (b *BatchCaretaker) stateStep(currentState BatchState) (BatchState, error)
case strings.Contains(err.Error(), "already exists"):
break

case err != nil:
default:
Copy link
Member

Choose a reason for hiding this comment

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

isn't this equiv to the above? A non-nil error ends up here w/ the prior version that had explicit err check. A commit body message could help here re rationale/understanding.

tapgarden/custodian.go
@@ -546,12 +546,15 @@ func (c *Custodian) importAddrToWallet(addr *address.AddrWithKeyInfo) error {
// along if so.
case strings.Contains(err.Error(), "already exists"):

case err != nil:
default:
Copy link
Member

Choose a reason for hiding this comment

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

Same here, or there's a condition that ptrAddr is nil but the error non-nil? Either way, seems equiv.

taprpc/mintrpc/mint.proto Outdated
@@ -33,40 +33,74 @@ service Mint {
rpc ListBatches (ListBatchRequest) returns (ListBatchResponse);
}

message MintAsset {
message MintSeedling {
Copy link
Member

Choose a reason for hiding this comment

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

IIUC, this will also trigger a larger breaking change as well (proto compilation failing). Seems non-essential?

taprpc/mintrpc/mint.proto Outdated Show resolved Hide resolved
taprpc/mintrpc/mint.proto Outdated
@@ -79,14 +113,20 @@ message MintAssetRequest {
If true, then the asset will be created with a group key, which allows for
future asset issuance.
*/
bool enable_emission = 2;
bool new_grouped_asset = 2;
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to not move this into MintAsset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was a reason when MintAsset was used within MintingBatch - now that that isn't the case, these could be moved into MintAsset.

taprpc/mintrpc/mint.proto Outdated
@@ -33,40 +33,74 @@ service Mint {
rpc ListBatches (ListBatchRequest) returns (ListBatchResponse);
}

message MintAsset {
message MintSeedling {
Copy link
Member

Choose a reason for hiding this comment

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

Looks like just for display, could be PendingBatchAsset, or just PendingAsset.

rpcserver.go Outdated Show resolved Hide resolved
rpcserver.go Outdated Show resolved Hide resolved
rpcserver.go Outdated Show resolved Hide resolved
@jharveyb
tapgarden: fix logging on addr import
64328da
@jharveyb
taprpc: add newGroupedAsset and GroupedAsset flags
5b1c032 
In this commit, we replace the enableEmission flag for minting with two
flags, newGroupedAsset and GroupedAsset. These are mutually exclusive
and less ambigious than the parsing of enableEmission. newGroupedAsset
is used when creating a group anchor, and groupedAsset is used when
issuing into an existing group. We also add the MintSeedling type to
better display the state of a pending batch, as the enableEmission
variable was missing before.
@jharveyb
rpcserver: use new asset group flags in MintAsset
dc2ef25
@jharveyb
itest: update mint requests + more negative tests
8cf481c
@jharveyb
tapcli: use new asset group flags in MintAsset
4fd858f
@jharveyb jharveyb force-pushed the enable_emission_flag_split branch from 1f43cf5 to 4fd858f Compare November 21, 2023 00:33
@jharveyb
Copy link
Contributor Author

Rebased + renamed MintSeedling to PendingAsset + renamed the last public EnableEmission field to NewGroupedAsset + moved the new asset group flags into MintAsset.

Roasbeef
Roasbeef approved these changes Nov 21, 2023
View reviewed changes
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🧜‍♀️

@Roasbeef Roasbeef merged commit c792a51 into main Nov 21, 2023
13 of 14 checks passed
@jharveyb jharveyb mentioned this pull request Nov 21, 2023
Closed
@jharveyb jharveyb deleted the enable_emission_flag_split branch November 27, 2023 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GeorgeTsagk GeorgeTsagk GeorgeTsagk left review comments

@ffranr ffranr ffranr approved these changes

@Roasbeef Roasbeef Roasbeef approved these changes

Assignees

@jharveyb jharveyb

Labels
cli
Projects
Taproot-Assets Project Board
Status: ✅ Done
Milestone
v0.3.2
Development

Successfully merging this pull request may close these issues.

add ability to set the emissions CLI flag to false
5 participants
@jharveyb @Roasbeef @ffranr @GeorgeTsagk @dstadulis