TransactionView: summary of transaction effects #4943
Conversation
|
|
||
| impl From<Balance> for pb::Balance { | ||
| fn from(_v: Balance) -> Self { | ||
| todo!() // todo: implement fallible conversion |
There was a problem hiding this comment.
comment: need to implement fallible conversion (proto to domain type)
| pub fn summary(&self) -> TransactionSummary { | ||
| let mut effects = Vec::new(); | ||
|
|
||
| for action_view in &self.body_view.action_views { |
There was a problem hiding this comment.
comment: Iterates through the visible action views and adds the contributing balances to the transaction's effects. Sanity check the required and provided balances are correct.
| ActionView::Swap(_) => todo!(), | ||
| ActionView::SwapClaim(_) => todo!(), |
There was a problem hiding this comment.
comment: need to implement other action views
| let amount = NonZeroU128::new(value.amount.into()) | ||
| .ok_or_else(|| anyhow::anyhow!("amount must be non-zero"))?; | ||
|
|
||
| let imbalance = Imbalance::Provided(amount); // todo: fix this placeholder |
There was a problem hiding this comment.
comment: Values cannot be negative, and a negative Balance is formed by negating a balance derived from a value. How do we calculate the Imbalance and differentiate between provided and required balances?
There was a problem hiding this comment.
I think the modeling suggested by henry here: https://github.com/penumbra-zone/penumbra/pull/4943/files#r1855950445 takes care of that
| pub struct Balance { | ||
| negated: bool, | ||
| #[serde_as(as = "Vec<(_, _)>")] |
There was a problem hiding this comment.
comment: confirmed the previous derived serde policy wasn't used.
| @@ -7,14 +7,15 @@ use anyhow::{Context, Error}; | |||
| use ark_ff::Zero; | |||
| use decaf377::Fr; | |||
| use decaf377_rdsa::{Binding, Signature, VerificationKey, VerificationKeyBytes}; | |||
| use penumbra_asset::Balance; | |||
| use penumbra_community_pool::{CommunityPoolDeposit, CommunityPoolOutput, CommunityPoolSpend}; | |||
| use penumbra_dex::{ | |||
| lp::action::{PositionClose, PositionOpen}, | |||
| swap::Swap, | |||
| }; | |||
There was a problem hiding this comment.
penumbra/crates/core/transaction/src/transaction.rs
Lines 735 to 739 in ff86c9a
comment: does this lead to infinite recursion?
| message Balance { | ||
| // Indicates if the balance is negated. | ||
| bool negated = 1; | ||
| // Represents the vector of 'Values' in the balance. | ||
| repeated Value balance = 2; | ||
| } |
There was a problem hiding this comment.
xref https://github.com/penumbra-zone/penumbra/pull/4943/files#r1855748563
Since Values can't be negative, I think we'll want to have a list of pairs, each with a negation flag, something like
message Balance {
message SignedValue {
Value value = 1;
bool negated = 2;
}
repeated SignedValue values = 1;
}
The top-level bool in the Rust Balance struct is an optimization that makes sign changes less expensive (unclear this was really necessary perf-wise, but, we have it now), IMO it should be excluded from serialization by having the serialized form always be "normalized" with individual sign bools.
| // Represents an individual effect of a transaction. | ||
| message Effects { | ||
| keys.v1.AddressView address = 1; | ||
| asset.v1.Balance balance = 2; | ||
| } |
There was a problem hiding this comment.
Let's move this into a sub-message so it's clear that it's logically scoped to be part of the TransactionSummary.
Describe your changes
References #4939
Issue ticket number and link
Checklist before requesting a review
I have added guiding text to explain how a reviewer should test these changes.
If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: