From ee5cc5901bdb5aeb2948d15e1bb2dea6fab6d976 Mon Sep 17 00:00:00 2001 From: Henry de Valence Date: Fri, 5 Apr 2024 13:46:55 -0400 Subject: [PATCH 1/4] proto: add extra metadata fields to `SwapView.Opaque` Many of the "opaque" variants of our view messages don't have any extra data other than the raw action. This turns out to be insufficient, because often there's additional data that's practically necessary to understand the public parts of the action. For instance, in this case, the `SwapView`, we want to correctly and nicely render the public parts of the swap, which include: - the input amounts, which are transparent; - the pro-rata output amounts, which are computable given public information. However, this information can't be easily rendered just from the underlying `Swap`, because: - it doesn't contain the asset `Metadata` about the values, only the amount and asset IDs; - it doesn't contain the `BatchSwapOutputData` with the (public) results of the batch swap; - it doesn't have precomputed output values, so the rendering would have to understand and replicate the pro-rata computations. Instead, we should make use of the perspective/view mechanism to augment the `SwapView.Opaque` with that data. To complete this PR, we'll need to: - [ ] Regenerate protos and fix any compile errors in the Rust code. - [ ] Ensure that the tx+perspective => view transformation uses data from the perspective to fill in the new fields (this should be similar to the existing code, but for opaque swaps rather than just visible ones). The web frontend's code for displaying simulated "public views" of the user's transactions can probably make use of this functionality after the first step (since it constructs the simulated view by attenuating the "full view" rather than by regenerating with a fresh perspective), but we should fill in the perspective generation for completeness and to ensure that the data we're adding actually makes sense and is possible to generate from the perspective. --- .../penumbra/core/component/dex/v1/dex.proto | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/proto/penumbra/penumbra/core/component/dex/v1/dex.proto b/proto/penumbra/penumbra/core/component/dex/v1/dex.proto index 88bf102fbf..53fd9d9167 100644 --- a/proto/penumbra/penumbra/core/component/dex/v1/dex.proto +++ b/proto/penumbra/penumbra/core/component/dex/v1/dex.proto @@ -156,6 +156,28 @@ message SwapView { message Opaque { dex.v1.Swap swap = 1; + // Optionally, if the swap has been confirmed, the batch price it received. + // + // As soon as the swap is detected, the view server can in principle record + // the relevant BSOD and provide it as part of the view. This allows providing + // info about the execution of the swap. + BatchSwapOutputData batch_swap_output_data = 20; + // Optionally, if the swap has been confirmed, the output value of asset 1. + // + // This is the value of the note that will be minted by the SwapClaim action. + // Note that unlike the `Visible` variant, this is only a `ValueView` since + // the details of the note (in particular the claim address) are not publicly known. + asset.v1.ValueView output_1_value = 30; + // Optionally, if the swap has been confirmed, the output value of asset 2. + // + // This is the note that will be minted by the SwapClaim action. + // Note that unlike the `Visible` variant, this is only a `ValueView` since + // the details of the note (in particular the claim address) are not publicly known. + asset.v1.ValueView output_2_value = 31; + // Optionally, metadata about asset 1 in the `swap`'s trading pair. + asset.v1.Metadata asset_1_metadata = 40; + // Optionally, metadata about asset 2 in the `swap`'s trading pair. + asset.v1.Metadata asset_2_metadata = 41; } oneof swap_view { From 10723ed201f32bccc31a06671f80064e511c4ec9 Mon Sep 17 00:00:00 2001 From: aubrey Date: Tue, 9 Apr 2024 15:48:14 -0700 Subject: [PATCH 2/4] regenerate protos --- .../src/gen/penumbra.core.component.dex.v1.rs | 35 ++++++++ .../penumbra.core.component.dex.v1.serde.rs | 90 +++++++++++++++++++ 2 files changed, 125 insertions(+) diff --git a/crates/proto/src/gen/penumbra.core.component.dex.v1.rs b/crates/proto/src/gen/penumbra.core.component.dex.v1.rs index 118449a51b..644a2c7922 100644 --- a/crates/proto/src/gen/penumbra.core.component.dex.v1.rs +++ b/crates/proto/src/gen/penumbra.core.component.dex.v1.rs @@ -305,6 +305,41 @@ pub mod swap_view { pub struct Opaque { #[prost(message, optional, tag = "1")] pub swap: ::core::option::Option, + /// Optionally, if the swap has been confirmed, the batch price it received. + /// + /// As soon as the swap is detected, the view server can in principle record + /// the relevant BSOD and provide it as part of the view. This allows providing + /// info about the execution of the swap. + #[prost(message, optional, tag = "20")] + pub batch_swap_output_data: ::core::option::Option, + /// Optionally, if the swap has been confirmed, the output value of asset 1. + /// + /// This is the value of the note that will be minted by the SwapClaim action. + /// Note that unlike the `Visible` variant, this is only a `ValueView` since + /// the details of the note (in particular the claim address) are not publicly known. + #[prost(message, optional, tag = "30")] + pub output_1_value: ::core::option::Option< + super::super::super::super::asset::v1::ValueView, + >, + /// Optionally, if the swap has been confirmed, the output value of asset 2. + /// + /// This is the note that will be minted by the SwapClaim action. + /// Note that unlike the `Visible` variant, this is only a `ValueView` since + /// the details of the note (in particular the claim address) are not publicly known. + #[prost(message, optional, tag = "31")] + pub output_2_value: ::core::option::Option< + super::super::super::super::asset::v1::ValueView, + >, + /// Optionally, metadata about asset 1 in the `swap`'s trading pair. + #[prost(message, optional, tag = "40")] + pub asset_1_metadata: ::core::option::Option< + super::super::super::super::asset::v1::Metadata, + >, + /// Optionally, metadata about asset 2 in the `swap`'s trading pair. + #[prost(message, optional, tag = "41")] + pub asset_2_metadata: ::core::option::Option< + super::super::super::super::asset::v1::Metadata, + >, } impl ::prost::Name for Opaque { const NAME: &'static str = "Opaque"; diff --git a/crates/proto/src/gen/penumbra.core.component.dex.v1.serde.rs b/crates/proto/src/gen/penumbra.core.component.dex.v1.serde.rs index a82e8bc664..b6efda7985 100644 --- a/crates/proto/src/gen/penumbra.core.component.dex.v1.serde.rs +++ b/crates/proto/src/gen/penumbra.core.component.dex.v1.serde.rs @@ -8215,10 +8215,40 @@ impl serde::Serialize for swap_view::Opaque { if self.swap.is_some() { len += 1; } + if self.batch_swap_output_data.is_some() { + len += 1; + } + if self.output_1_value.is_some() { + len += 1; + } + if self.output_2_value.is_some() { + len += 1; + } + if self.asset_1_metadata.is_some() { + len += 1; + } + if self.asset_2_metadata.is_some() { + len += 1; + } let mut struct_ser = serializer.serialize_struct("penumbra.core.component.dex.v1.SwapView.Opaque", len)?; if let Some(v) = self.swap.as_ref() { struct_ser.serialize_field("swap", v)?; } + if let Some(v) = self.batch_swap_output_data.as_ref() { + struct_ser.serialize_field("batchSwapOutputData", v)?; + } + if let Some(v) = self.output_1_value.as_ref() { + struct_ser.serialize_field("output1Value", v)?; + } + if let Some(v) = self.output_2_value.as_ref() { + struct_ser.serialize_field("output2Value", v)?; + } + if let Some(v) = self.asset_1_metadata.as_ref() { + struct_ser.serialize_field("asset1Metadata", v)?; + } + if let Some(v) = self.asset_2_metadata.as_ref() { + struct_ser.serialize_field("asset2Metadata", v)?; + } struct_ser.end() } } @@ -8230,11 +8260,26 @@ impl<'de> serde::Deserialize<'de> for swap_view::Opaque { { const FIELDS: &[&str] = &[ "swap", + "batch_swap_output_data", + "batchSwapOutputData", + "output_1_value", + "output1Value", + "output_2_value", + "output2Value", + "asset_1_metadata", + "asset1Metadata", + "asset_2_metadata", + "asset2Metadata", ]; #[allow(clippy::enum_variant_names)] enum GeneratedField { Swap, + BatchSwapOutputData, + Output1Value, + Output2Value, + Asset1Metadata, + Asset2Metadata, __SkipField__, } impl<'de> serde::Deserialize<'de> for GeneratedField { @@ -8258,6 +8303,11 @@ impl<'de> serde::Deserialize<'de> for swap_view::Opaque { { match value { "swap" => Ok(GeneratedField::Swap), + "batchSwapOutputData" | "batch_swap_output_data" => Ok(GeneratedField::BatchSwapOutputData), + "output1Value" | "output_1_value" => Ok(GeneratedField::Output1Value), + "output2Value" | "output_2_value" => Ok(GeneratedField::Output2Value), + "asset1Metadata" | "asset_1_metadata" => Ok(GeneratedField::Asset1Metadata), + "asset2Metadata" | "asset_2_metadata" => Ok(GeneratedField::Asset2Metadata), _ => Ok(GeneratedField::__SkipField__), } } @@ -8278,6 +8328,11 @@ impl<'de> serde::Deserialize<'de> for swap_view::Opaque { V: serde::de::MapAccess<'de>, { let mut swap__ = None; + let mut batch_swap_output_data__ = None; + let mut output_1_value__ = None; + let mut output_2_value__ = None; + let mut asset_1_metadata__ = None; + let mut asset_2_metadata__ = None; while let Some(k) = map_.next_key()? { match k { GeneratedField::Swap => { @@ -8286,6 +8341,36 @@ impl<'de> serde::Deserialize<'de> for swap_view::Opaque { } swap__ = map_.next_value()?; } + GeneratedField::BatchSwapOutputData => { + if batch_swap_output_data__.is_some() { + return Err(serde::de::Error::duplicate_field("batchSwapOutputData")); + } + batch_swap_output_data__ = map_.next_value()?; + } + GeneratedField::Output1Value => { + if output_1_value__.is_some() { + return Err(serde::de::Error::duplicate_field("output1Value")); + } + output_1_value__ = map_.next_value()?; + } + GeneratedField::Output2Value => { + if output_2_value__.is_some() { + return Err(serde::de::Error::duplicate_field("output2Value")); + } + output_2_value__ = map_.next_value()?; + } + GeneratedField::Asset1Metadata => { + if asset_1_metadata__.is_some() { + return Err(serde::de::Error::duplicate_field("asset1Metadata")); + } + asset_1_metadata__ = map_.next_value()?; + } + GeneratedField::Asset2Metadata => { + if asset_2_metadata__.is_some() { + return Err(serde::de::Error::duplicate_field("asset2Metadata")); + } + asset_2_metadata__ = map_.next_value()?; + } GeneratedField::__SkipField__ => { let _ = map_.next_value::()?; } @@ -8293,6 +8378,11 @@ impl<'de> serde::Deserialize<'de> for swap_view::Opaque { } Ok(swap_view::Opaque { swap: swap__, + batch_swap_output_data: batch_swap_output_data__, + output_1_value: output_1_value__, + output_2_value: output_2_value__, + asset_1_metadata: asset_1_metadata__, + asset_2_metadata: asset_2_metadata__, }) } } From b2889961b3412df43195c7bdf688c5e3b6c0c092 Mon Sep 17 00:00:00 2001 From: aubrey Date: Tue, 9 Apr 2024 16:36:24 -0700 Subject: [PATCH 3/4] Populate additional Swap fields for Opaque variants --- crates/bin/pcli/src/transaction_view_ext.rs | 2 +- crates/core/component/dex/src/swap/view.rs | 31 ++++++++++- crates/core/transaction/src/is_action.rs | 62 ++++++++++++++++++++- 3 files changed, 88 insertions(+), 7 deletions(-) diff --git a/crates/bin/pcli/src/transaction_view_ext.rs b/crates/bin/pcli/src/transaction_view_ext.rs index c2ed4b1c2c..ce1288d1e3 100644 --- a/crates/bin/pcli/src/transaction_view_ext.rs +++ b/crates/bin/pcli/src/transaction_view_ext.rs @@ -254,7 +254,7 @@ impl TransactionViewExt for TransactionView { ["Swap", &action] } - SwapView::Opaque { swap } => { + SwapView::Opaque { swap, .. } => { action = format!( "Opaque swap for trading pair: {} <=> {}", format_asset_id(&swap.body.trading_pair.asset_1()), diff --git a/crates/core/component/dex/src/swap/view.rs b/crates/core/component/dex/src/swap/view.rs index 31c96a54f5..db50783eeb 100644 --- a/crates/core/component/dex/src/swap/view.rs +++ b/crates/core/component/dex/src/swap/view.rs @@ -1,4 +1,4 @@ -use penumbra_asset::asset::Metadata; +use penumbra_asset::{asset::Metadata, ValueView}; use penumbra_proto::{penumbra::core::component::dex::v1 as pb, DomainType}; use penumbra_shielded_pool::NoteView; use penumbra_txhash::TransactionId; @@ -24,6 +24,11 @@ pub enum SwapView { }, Opaque { swap: Swap, + batch_swap_output_data: Option, + output_1: Option, + output_2: Option, + asset_1_metadata: Option, + asset_2_metadata: Option, }, } @@ -63,6 +68,14 @@ impl TryFrom for SwapView { .swap .ok_or_else(|| anyhow::anyhow!("missing swap field"))? .try_into()?, + batch_swap_output_data: x + .batch_swap_output_data + .map(TryInto::try_into) + .transpose()?, + output_1: x.output_1_value.map(TryInto::try_into).transpose()?, + output_2: x.output_2_value.map(TryInto::try_into).transpose()?, + asset_1_metadata: x.asset_1_metadata.map(TryInto::try_into).transpose()?, + asset_2_metadata: x.asset_2_metadata.map(TryInto::try_into).transpose()?, }), } } @@ -93,9 +106,21 @@ impl From for pb::SwapView { batch_swap_output_data: batch_swap_output_data.map(Into::into), })), }, - SwapView::Opaque { swap } => Self { + SwapView::Opaque { + swap, + batch_swap_output_data, + output_1, + output_2, + asset_1_metadata, + asset_2_metadata, + } => Self { swap_view: Some(sv::SwapView::Opaque(sv::Opaque { swap: Some(swap.into()), + batch_swap_output_data: batch_swap_output_data.map(Into::into), + output_1_value: output_1.map(Into::into), + output_2_value: output_2.map(Into::into), + asset_1_metadata: asset_1_metadata.map(Into::into), + asset_2_metadata: asset_2_metadata.map(Into::into), })), }, } @@ -106,7 +131,7 @@ impl From for Swap { fn from(v: SwapView) -> Self { match v { SwapView::Visible { swap, .. } => swap, - SwapView::Opaque { swap } => swap, + SwapView::Opaque { swap, .. } => swap, } } } diff --git a/crates/core/transaction/src/is_action.rs b/crates/core/transaction/src/is_action.rs index 8cfa077e7b..1967b9d4a4 100644 --- a/crates/core/transaction/src/is_action.rs +++ b/crates/core/transaction/src/is_action.rs @@ -367,9 +367,65 @@ impl IsAction for Swap { .cloned(), } } - None => SwapView::Opaque { - swap: self.to_owned(), - }, + None => { + // If we can find a matching BSOD in the TxP, we can use it to compute the output notes + // for the swap. + let bsod = txp + .batch_swap_output_data + .iter() + // This finds the first matching one; there should only be one + // per trading pair per block and we trust the TxP provider not to lie about it. + .find(|bsod| bsod.trading_pair == self.body.trading_pair); + + // We can get the denom metadata whether we get a BSOD or not + + let denom_1 = txp.denoms.get(&self.body.trading_pair.asset_1()).cloned(); + let denom_2 = txp.denoms.get(&self.body.trading_pair.asset_2()).cloned(); + + match bsod { + None => { + // If we can't find a matching BSOD, we can't compute the output notes + // for the swap. + // TODO: is defaulting to 'None' the right behavior here, or is not matching on a BSOD as above an error? + SwapView::Opaque { + swap: self.to_owned(), + batch_swap_output_data: None, + output_1: None, + output_2: None, + asset_1_metadata: denom_1.clone(), + asset_2_metadata: denom_2.clone(), + } + } + Some(bsod) => { + // If we can find a matching BSOD, use it to compute the output notes + // for the swap. + + let (lambda_1_i, lambda_2_i) = + bsod.pro_rata_outputs((self.body.delta_1_i, self.body.delta_2_i)); + + SwapView::Opaque { + swap: self.to_owned(), + batch_swap_output_data: Some(bsod.clone()), + asset_1_metadata: denom_1.clone(), + asset_2_metadata: denom_2.clone(), + output_1: Some( + Value { + amount: lambda_1_i, + asset_id: self.body.trading_pair.asset_1(), + } + .view_with_cache(&txp.denoms), + ), + output_2: Some( + Value { + amount: lambda_2_i, + asset_id: self.body.trading_pair.asset_2(), + } + .view_with_cache(&txp.denoms), + ), + } + } + } + } }) } } From 5d3533f4b16fbe3f18599b7f9352a6eb0f084b97 Mon Sep 17 00:00:00 2001 From: katelyn martin Date: Mon, 22 Apr 2024 15:24:22 -0400 Subject: [PATCH 4/4] transaction: remove `TODO` comment this removes a todo comment asking a question. > Not finding a BSOD in the TxP means that whoever generated the > transaction perspective didn't include that data, either because they > didn't have it (eg the transaction was created but never confirmed by > the chain and executed) or because they didn't support it in their > implementation. > > We should handle that by graceful degradation, just not rendering the > BSOD-requiring data if we don't have it. \- https://github.com/penumbra-zone/penumbra/pull/4164#discussion_r1575246052 --- crates/core/transaction/src/is_action.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/crates/core/transaction/src/is_action.rs b/crates/core/transaction/src/is_action.rs index 1967b9d4a4..65766463cd 100644 --- a/crates/core/transaction/src/is_action.rs +++ b/crates/core/transaction/src/is_action.rs @@ -378,7 +378,6 @@ impl IsAction for Swap { .find(|bsod| bsod.trading_pair == self.body.trading_pair); // We can get the denom metadata whether we get a BSOD or not - let denom_1 = txp.denoms.get(&self.body.trading_pair.asset_1()).cloned(); let denom_2 = txp.denoms.get(&self.body.trading_pair.asset_2()).cloned(); @@ -386,7 +385,6 @@ impl IsAction for Swap { None => { // If we can't find a matching BSOD, we can't compute the output notes // for the swap. - // TODO: is defaulting to 'None' the right behavior here, or is not matching on a BSOD as above an error? SwapView::Opaque { swap: self.to_owned(), batch_swap_output_data: None, @@ -399,10 +397,8 @@ impl IsAction for Swap { Some(bsod) => { // If we can find a matching BSOD, use it to compute the output notes // for the swap. - let (lambda_1_i, lambda_2_i) = bsod.pro_rata_outputs((self.body.delta_1_i, self.body.delta_2_i)); - SwapView::Opaque { swap: self.to_owned(), batch_swap_output_data: Some(bsod.clone()),