-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
feat(bank): reserve proto field numbers used by Penumbra #19298
Conversation
Penumbra does not use the Cosmos SDK, but it does intend to interoperate with it. One point of interoperability is in asset metadata. The SDK provides a `bank.Metadata` message with metadata about an asset. Penumbra has a corresponding metadata message whose fields are a strict superset of the `bank.Metadata` message, so that Penumbra can parse SDK assets and vice versa. This commit reserves two fields, by number and name, in order to ensure this compatibility can extend into the future. First, it reserves a `penumbra_asset_id` field, numbered `1984`. Unlike the SDK, which identifies tokens by a "base denom" using the bank module, Penumbra identifies tokens using an "asset ID", an element of the finite field we use for SNARK proofs. For SDK-compatible assets, the asset ID is computed as a hash of the base denom string. Penumbra clients need to be able to match asset IDs with metadata, so we extended the metadata spec with an asset ID, and chose a name and field number we thought would be unlikely to conflict with the SDK. The compatibility story is as follows: since the asset ID can be computed from the base denom, Penumbra's Rust libraries can parse SDK metadata without problems, computing the asset ID if it is missing and filling it in. And since proto parsers are expected to ignore unknown fields, SDK tooling can simply ignore and discard the unknown field. (Why, then, include it at all? Because computing the asset ID involves Penumbra-specific cryptography, which may not be easily accessible in every language wishing to work with Penumbra data, such as Typescript). Reserving this name and field number ensures that Penumbra and the SDK remain compatible going forward. Second, it reserves an `images` field, numbered `1985`. This corresponds to the `images` structure defined in the cosmos asset registry: https://github.com/cosmos/chain-registry/blob/42d163653ba1d32397d4dea48611c4b11f4c7e82/assetlist.schema.json#L136-L175 That repo notes that it is intended to be a strict superset of the SDK's proto definitions, to permit the possibility of upstreaming into the SDK at some future point. However, Penumbra's client tooling is entirely proto-based, so we cannot make use of the fields without upstreaming them into our protos, which we did. Because the json schema only specifies a name, not a field number, we had to choose a field number when we upstreamed the definitions. We chose 1985, to follow our previous extension. Reserving the `images` field name with the same field number as Penumbra used ensures that, in the event that the SDK chose to upstream this extension into its protos, we would retain Penumbra<>SDK data format compatibility for both ProtoJSON and binary Protobuf encodings.
WalkthroughThe recent modifications to the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
This commit is incomplete, in particular I don't know how the SDK's proto tooling works and didn't try to update. It's intended as a concrete starting point for discussion. |
this seems a bit hacky to make "it work well enough". If we want to do this correctly we will need to identify if we should change this structure for sdk users then be able to get back to this. Images here seems like something we should allow users to add. Lets talk to users and agree on overall changes that need to happen to this structure. The chain registry has evolved on it own due to lack of coordination on many fronts. We should try to align these sort of things. |
I'm not sure I understand, allowing users to add images is exactly the point of this change! To clarify, the goal is not that every on-chain instance of a As a concrete example, here's how we'll be using this in Penumbra. (Remember that all proto fields are optional, so they can be left unset).
The key points here are:
|
// Used by the cosmos asset registry to record images (logos), in json-schema extensions to this message. | ||
// Reserved here to align with Penumbra's upstreaming of those extensions into its proto definitions. | ||
reserved 1985; | ||
reserved "images"; |
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 think this is fine, but i cant guarantee we or some other team may change it in the future
Could you fix the conflicts and run |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Penumbra does not use the Cosmos SDK, but it does intend to interoperate with it. One point of interoperability is in asset metadata. The SDK provides a
bank.Metadata
message with metadata about an asset. Penumbra has a corresponding metadata message whose fields are a strict superset of thebank.Metadata
message, so that Penumbra can parse SDK assets and vice versa.This commit reserves two fields, by number and name, in order to ensure this compatibility can extend into the future.
First, it reserves a
penumbra_asset_id
field, numbered1984
.Unlike the SDK, which identifies tokens by a "base denom" using the bank module, Penumbra identifies tokens using an "asset ID", an element of the finite field we use for SNARK proofs. For SDK-compatible assets, the asset ID is computed as a hash of the base denom string. Penumbra clients need to be able to match asset IDs with metadata, so we extended the metadata spec with an asset ID, and chose a name and field number we thought would be unlikely to conflict with the SDK.
The compatibility story is as follows: since the asset ID can be computed from the base denom, Penumbra's Rust libraries can parse SDK metadata without problems, computing the asset ID if it is missing and filling it in. And since proto parsers are expected to ignore unknown fields, SDK tooling can simply ignore and discard the unknown field.
(Why, then, include it at all? Because computing the asset ID involves Penumbra-specific cryptography, which may not be easily accessible in every language wishing to work with Penumbra data, such as Typescript).
Reserving this name and field number ensures that Penumbra and the SDK remain compatible going forward.
Second, it reserves an
images
field, numbered1985
.This corresponds to the
images
structure defined in the cosmos asset registry:https://github.com/cosmos/chain-registry/blob/42d163653ba1d32397d4dea48611c4b11f4c7e82/assetlist.schema.json#L136-L175
That repo notes that it is intended to be a strict superset of the SDK's proto definitions, to permit the possibility of upstreaming into the SDK at some future point. However, Penumbra's client tooling is entirely proto-based, so we cannot make use of the fields without upstreaming them into our protos, which we did. Because the json schema only specifies a name, not a field number, we had to choose a field number when we upstreamed the definitions. We chose 1985, to follow our previous extension.
Reserving the
images
field name with the same field number as Penumbra used ensures that, in the event that the SDK chose to upstream this extension into its protos, we would retain Penumbra<>SDK data format compatibility for both ProtoJSON and binary Protobuf encodings.