Correct derivation of Penumbra asset ID #4
Conversation
Rather than manually computing the asset ID, we use the existing `penumbra_asset::asset::Metadata` structure to derive it from the metadata. This also ensures that we're running our transported `Metadata` objects through the (de)serialization code we'll eventually use to process them in the client.
hdevalence
force-pushed
the
improve-metadata-handling
branch
from
46cd87e to
5ac5062
Compare
| // Prefix the channel to the base denom. | ||
| prefix_channel(&mut pb_metadata.base); | ||
| // Prefix the channel to the display denom. | ||
| prefix_channel(&mut pb_metadata.display); | ||
| // Prefix the channel to all denom units. | ||
| for denom_unit in pb_metadata.denom_units.iter_mut() { | ||
| prefix_channel(&mut denom_unit.denom); | ||
| } |
There was a problem hiding this comment.
We should apply the prefix across all denoms and units. Also, I think the Rust Metadata parsing will reject if the base and display don't index into denom_units.
There was a problem hiding this comment.
Ah, we seemed to miss this in our current registry too: https://github.com/penumbra-zone/web/blob/main/packages/constants/src/local-asset-registry.json#L176-L188. Good to know.
|
We should check this against the actual IBC data. |
| let asset_json = serde_json::to_string(&source_asset)?; | ||
| let transferred_asset = | ||
| transport_metadata_along_channel(&ibc_data, serde_json::from_str(&asset_json)?); | ||
| tracing::info!(?asset_json, transferred_asset_json = ?serde_json::to_string(&transferred_asset)); |
There was a problem hiding this comment.
Is this outputting something helpful for you? I just see what feels like a bunch of noise. But maybe it's me not familiar with this tracing library.
There was a problem hiding this comment.
Yeah I was using it to inspect the JSON output and determine how the encodings differed during debugging. We can remove
| pb_metadata.penumbra_asset_id = None; | ||
|
|
||
| tracing::debug!(?pb_metadata, "new"); | ||
| Metadata::try_from(pb_metadata).unwrap() |
There was a problem hiding this comment.
Think we should return a result here. I'll get this in a later PR though.
There was a problem hiding this comment.
Don't think so, if we had valid Metadata to start, our modifications should also be valid, or else the compiler should fail.
| // Prefix the channel to the base denom. | ||
| prefix_channel(&mut pb_metadata.base); | ||
| // Prefix the channel to the display denom. | ||
| prefix_channel(&mut pb_metadata.display); | ||
| // Prefix the channel to all denom units. | ||
| for denom_unit in pb_metadata.denom_units.iter_mut() { | ||
| prefix_channel(&mut denom_unit.denom); | ||
| } |
There was a problem hiding this comment.
Ah, we seemed to miss this in our current registry too: https://github.com/penumbra-zone/web/blob/main/packages/constants/src/local-asset-registry.json#L176-L188. Good to know.
| // Prefix the channel to the base denom. | ||
| prefix_channel(&mut pb_metadata.base); | ||
| // Prefix the channel to the display denom. | ||
| prefix_channel(&mut pb_metadata.display); |
There was a problem hiding this comment.
Do we indeed want to show transfer/channel-0/usomo in our UI if we believe it's the canonical one? I believe minifront is using display in a few places 🤔 . I suppose if we rely on symbol, this wouldn't be an issue anyway. Though, a native assets on two different connected chains could in theory conflict on symbol.
There was a problem hiding this comment.
Yeah I think we should use this as a forcing function to only ever use the symbol field in normie UI
| @@ -74,5 +74,6 @@ pub fn get_chain_configs() -> AppResult<Vec<ChainConfig>> { | |||
| }; | |||
| Ok(chain_config) | |||
| }) | |||
| .collect() | |||
| .filter_map(|result| result.ok()) | |||
There was a problem hiding this comment.
Don't we want to throw if the user did not set up the file structure correctly? Wouldn't this mean it kinda would silently fail?
There was a problem hiding this comment.
Yeah, I did this along the way to starting towards having it consume a git submodule and change the input directory structure, which I abandoned and then wrote up as a follow on issue. Getting output of some kind felt (feels) more important
Rather than manually computing the asset ID, we use the existing
penumbra_asset::asset::Metadatastructure to derive it from the metadata. This also ensures that we're running our transportedMetadataobjects through the (de)serialization code we'll eventually use to process them in the client.