proof: add new universe commitment TLV fields to asset meta reveal #1335
Conversation
guggero
force-pushed
the
tlv-decimal-display
branch
from
9180e85 to
e037590
Compare
Pull Request Test Coverage Report for Build 13071869152Details
💛 - Coveralls |
There was a problem hiding this comment.
I haven't reviewed these changes but:
I've been thinking more about universe commitments. I think we should get further along in the design of that feature before deciding on a universe commitment asset metadata TLV field. It might be wise to split out the universe commitment field from this PR. We can catchup on this.
guggero
force-pushed
the
tlv-decimal-display
branch
from
e037590 to
f81318f
Compare
guggero
force-pushed
the
tlv-decimal-display
branch
from
f81318f to
56e113e
Compare
|
@ffranr I've removed the |
tapgarden/seedling.go
Outdated
| // We also require the canonical universe URL and universe commitment | ||
| // flag to match. | ||
| var ( | ||
| seedlingCanonicalURL string | ||
| anchorCanonicalURL string | ||
| ) | ||
| if seedlingMeta != nil && seedlingMeta.CanonicalUniverse != nil { | ||
| seedlingCanonicalURL = seedlingMeta.CanonicalUniverse.String() | ||
| } | ||
| if anchorMeta != nil && anchorMeta.CanonicalUniverse != nil { | ||
| anchorCanonicalURL = anchorMeta.CanonicalUniverse.String() | ||
| } | ||
|
|
||
| if seedlingCanonicalURL != anchorCanonicalURL { | ||
| return fmt.Errorf("seedling canonical universe URL does not "+ | ||
| "match group anchor: %v, %v", seedlingCanonicalURL, | ||
| anchorCanonicalURL) | ||
| } | ||
|
|
There was a problem hiding this comment.
At this point, I think, that the anchor meta will be the genesis asset meta. (I check validateGroupKey call site).
This check in the PR enforces that the canonical universe URL for the new seedling matches the one specified in the genesis asset. I don’t think we should impose this restriction—a new minting tranche should be allowed to define its own canonical universe URL.
With that allowance, the minter can update the canonical universe URL.
There was a problem hiding this comment.
I don't necessarily disagree with potentially having the option to update the URL at a later point.
But since we don't know yet how that would affect client code (e.g. when syncing a grouped asset's issuance history, would we need to follow the whole chain to the very last mint to find out what URL to use?), I think we should keep this restriction for now.
This will also help make sure the user doesn't forget to set the URL by accident. Or has a typo on a subsequent mint.
There was a problem hiding this comment.
This will also help make sure the user doesn't forget to set the URL by accident. Or has a typo on a subsequent mint.
Isn't that precisely why we should allow updating the URL in a subsequent tranche? What’s our current plan if a user mints an asset on-chain and later discovers the URL is incorrect? (We've encountered a similar issue with the proof courier URL in tap addresses.)
Maybe we should consider a universe server (if that's the URL scheme) connection check as part of minting.
From my perspective, when a client attempts to evaluate a mint and has access to a single asset ID within a given group, they also have three potential canonical universe URLs:
- The canonical URL of the tranche they are evaluating.
- The genesis canonical URL.
- The default URL hardcoded into
tapd.
In my opinion, tapd should sync with these URLs in the specified order. Having access to multiple URLs, rather than relying solely on the genesis URL, doesn’t seem problematic. From my perspective, the purpose of the canonical URL field is to enhance the likelihood of a successful sync.
There was a problem hiding this comment.
I see your point. Not sure about the connection check, could be a bit of a hurdle (perhaps you first want to make sure the mint succeeds before setting up the universe server?), I'd suggest adding that to the PR that adds the CLI/RPC flags for the field in the first place.
Good idea about syncing in order and with decreasing priority if there are multiple sources for a potential canonical universe URL. Going to remove the check for now then.
With this commit we add the decimal display as a new TLV field on the meta reveal. We'll still encode it within the JSON meta data if the meta data is of type JSON. For all other types we can now also carry the value, to make sure we don't put any type constraints on future asset mints (in case issuers want to use a different format than JSON). Since we're using TLV, old meta reveals should still be read correctly and the default value of 0 is assumed for any meta reveal that doesn't have the explicit record set. We'll add a unit test for that in the next commit.
With this commit we make sure that a new seedling that attempts to mint into the same group as a previous mint has the same meta fields.
guggero
force-pushed
the
tlv-decimal-display
branch
from
56e113e to
9a2d3c6
Compare
|
|
||
| require.Len(t.t, thirdAssets, 1) | ||
| require.NotNil(t.t, thirdAssets[0].DecimalDisplay) | ||
| require.EqualValues( |
There was a problem hiding this comment.
Should we also assert that the dec display is still encoded in the asset meta JSON here (still getting through the PR).
There was a problem hiding this comment.
Also what should be the behavior if the user sets the field manually in the JSON meta? What about if that value differs from the one here in the proto.
| // If the decimal display is set as the new TLV value, we can use that | ||
| // directly. | ||
| if decimalDisplay, isSet := m.DecimalDisplay.Unwrap(); isSet { | ||
| return nil, decimalDisplay, nil |
There was a problem hiding this comment.
Before returning, should we do a sanity check that:
- The value isn't already encoded in the asset meta JSON
- That if it is then it matches this value
?
| // If a custom decimal display is set, the meta type must also be set to | ||
| // JSON. | ||
| // If a custom decimal display is set, we require the AssetMeta to be | ||
| // set. That means the user has to at least specify the meta type. |
There was a problem hiding this comment.
What about assuming it's just the opaque type if the dec display is set? Given that by default (opaque asset type, not JSON), we won't encode this in the asset meta.
There was a problem hiding this comment.
Hmm, yeah, I guess we could do that now. But that would mean that both the meta data blob and type would be empty/unset and the meta hash would just be TLV record of the decimal display?
| // CanonicalUniverse is the URL of the canonical (approved, | ||
| // authoritative) universe where the asset minting and universe | ||
| // commitment proofs will be pushed to. | ||
| CanonicalUniverse fn.Option[url.URL] |
There was a problem hiding this comment.
What about instead lifting this to the issuance proof itself? We can require that this be signed by the group key so it can't just be swapped out by a 3rd party.
This way, one can update this filed with a new issuance event (or some sort of off-chain proof), and not be locked in from the very start (DNS addr changes, etc).
Thinking about it further, we can even tie this into the unified universe commitment structure. We can add a 4th commitment/field, that can be updated via a spend on chain to distribute new meta/param updates to all the verifiers.
There was a problem hiding this comment.
Another idea is we just put an authentication/delegation key here. This key can be used in the pre-commitment output, and then can also be used to delegate authentication capabilities to another key. Such abilities can include updating the canonical universe value.
There was a problem hiding this comment.
Also see discussion here: #1335 (comment)
I guess at this point the field in the meta reveal would be more of a "universe hint" than the "authoritative" universe. The use for it would mostly be bootstrapping:
- New
tapdcomes online, only syncs to the default universe (e.g. the hard coded one) - User wants to learn about grouped asset X, turns on issuance proof sync for just asset X
tapdsyncs the issuance proofs for asset X, sees universe URL in meta reveal of those syncstapdturns on universe commitment sync for asset X using the universe URL(s) found in the meta reveals
But maybe you have a different flow in mind... Should we just remove the field then if we're not really sure yet how we're going to use it?
Another idea is we just put an authentication/delegation key here.
We are going to use an authenticated key for the pre-commitment key. So perhaps we could also include the universe URL in the actual universe commitment proof, where it would be more "authoritative" and could be updated. But IMO that URL can't only be in the universe commitment proof itself, otherwise you we have a bootstrap issue (you can only get the universe commitment proofs from the universe that then tells you what URL to use to get universe commitment proofs...)
There was a problem hiding this comment.
We can require that this be signed by the group key so it can't just be swapped out by a 3rd party.
Not sure if that's a good idea UX wise. In case of a cold wallet mint, you'd already need to sign at least 2 PSBTs per mint event (one for the group key VM TX, one spending the previous universe commitment output). So needing to sign yet another transaction just makes things clunky IMO.
There was a problem hiding this comment.
I guess at this point the field in the meta reveal would be more of a "universe hint" than the "authoritative" universe.
The way I've been viewing things is that this is the primary Universe that should be checked on spend to obtain the pre-image of the commitment data, which is this new unified commitment. It should be what people check tomake sure they have all the data, but ofc this data can still be mirrored elsewhere.
tapd syncs the issuance proofs for asset X, sees universe URL in meta reveal of those syncs
tapd turns on universe commitment sync for asset X using the universe URL(s) found in the meta reveals
So with my suggestion, rather than seeing it in the meta, they'd see it in a singed section in the proof metadata. To go a step further, we might also augment the genesis virtual tx to commit to this data.
One thing that we still haven't spec'd out, is the RPC API for fetching this new unified commitment root, the sub-trees and the leaves of those sub-trees. The "normal" Universes, may not offer this specialized API, though to be maximally useful, they should mirror it from the canonical universe.
So perhaps we could also include the universe URL in the actual universe commitment proof, where it would be more "authoritative" and could be updated
This sounds similar to the idea I mentioned earlier about having a param sub-tree/commitment. This is effectively authenticated, as it can only be updated by being able to spend the pre-commitment tree.
However, as verifiers will first sync the normal issuance proofs, we need to provide them with enough information to then be able to locate + authenticate the canonical Universe. The issuance proofs can include a signed payload that contains this bootstrap + discovery information (including what key to use to verify.
But IMO that URL can't only be in the universe commitment proof itself, otherwise you we have a bootstrap issue (you can only get the universe commitment proofs from the universe that then tells you what URL to use to get universe commitment proofs...)
I don't see how a bootstrap issue arises. You use the existing normal Universes to fetch the issuance proofs, etc. But these may not have burn, and the new meta/param tree we've been ideating. To be able to properly verify the commitment spends on chain, you look to this canonical Universe.
There was a problem hiding this comment.
Not sure if that's a good idea UX wise. In case of a cold wallet mint, you'd already need to sign at least 2 PSBTs per mint event (one for the group key VM TX, one spending the previous universe commitment output). So needing to sign yet another transaction just makes things clunky IMO.
Agreed. So one way we can avoid that clunkiness is allowing a delegation key to be presented in the initial meta. This key is then the one that signs the set of booststrap params, and can even include yet another delegation key in the set of params, etc.
|
Bringing this outside the thread so it doesn't get lost. Here's my idea w.r.t what should go into the meta, how bootstrapping would work, and this new bootstrap params field:
|
|
Trying to summarize this thread and combine it with the internal design doc. Initial mint
Subsequent mint
Here's my original version I posted with some questions that were then discussed offline.
Old versionInitial mint
Subsequent mint
My main question here is: Do we really need two different delegation keys? One in the asset metadata and then a new one in the issuance proof's commitment params? I only differentiate between those two keys because @Roasbeef mentioned two delegation keys above. But I think this could be the same one? So just one delegation key defined in the asset meta that signs for both the issuance proof payload and the universe commitment payload. Second question: Do we still want to lock the actual universe commitment transaction output to the group key (plus a leaf that contains the commitment roots)? Or would that also lock to the delegated key? |
|
I think I pretty much agree with Oli's summary. Except that I don't think we should ever lock any uni commitment related output with the group key. Because of the external cold key signing requirements. And I don't see the point in two different delegation keys also. I think the canonical universe URL should go in the issuance proof (signed payload) and then we should be able to update it in the universe commitment proof. So I don't think the bootstrapping issue is a blocker.
I agree with this sort of idea. I think we should generalise the whole universe commitment notion to something that the minter can use to issue general updates (update canonical uni URL, update ignore list, update total issuance, etc.) |
|
Just thinking outloud: I think what we need in the asset meta data is:
And then in the signed payload in the issuance proof we can have:
And then the signature field for the payload. |
|
I updated my comment above with what we discussed. Thanks for your inputs! |
|
I don't think the To be clear, I'm think of a minter who:
If we force the URL to be in the issuance proof signed payload they will have to manage a delegation key (at least for now?), which they don't necessarily need to manage. I guess it comes down to: is specifying canonical URL and not enabling uni commitments reasonable? And I think the answer is yes. |
Depends on #1337.Fixes #956.
Adds two new fields to the TLV record of the asset's meta reveal:
Important for reviewers:
While writing this PR a bug was found in the backward compatible way we use odd TLV records. See e9286f0. Unfortunately that means these new fields will only be backward compatible tov0.5.1(this PR) and not all the way back tov0.5.0.We prepare for future backward compatibility tests with 293c60f. The idea is to be able to have a CI step that check out an older version (e.g.v0.5.1) in the future and run that test with updated test vectors of the future version, to make sure they can still be parsed and validated by that older version.