Skip to content
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.

Sign up for GitHub

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

Jump to bottom

implement TransactionView #1426

Merged
merged 3 commits into from
Oct 3, 2022
Merged

implement TransactionView #1426

merged 3 commits into from
Oct 3, 2022

Conversation

redshiftzero
Copy link
Member

@redshiftzero redshiftzero commented Sep 14, 2022 •
edited by aubrika

@aubrika and I started sketching out the TransactionView required for implementing pcli v tx as described in #1364

@redshiftzero redshiftzero temporarily deployed to smoke-test September 14, 2022 17:35 Inactive
@aubrika aubrika mentioned this pull request Sep 14, 2022
Closed
17 tasks
@aubrika
Copy link
Contributor

aubrika commented Sep 14, 2022

As mentioned in #1368, it looks like there are just four Action types that will require additional data not present in the Transaction object itself (for now). @hdevalence does this look like we are heading in the right direction here based on the stubbed decryption code?

@aubrika aubrika temporarily deployed to smoke-test September 20, 2022 19:38 Inactive
@aubrika aubrika changed the title wip: initial work towards TransactionView wip: implementTransactionView and pcli v tx Sep 20, 2022
@aubrika aubrika changed the title wip: implementTransactionView and pcli v tx wip: implement TransactionView and pcli v tx Sep 20, 2022
@aubrika aubrika temporarily deployed to smoke-test September 21, 2022 18:18 Inactive
@aubrika aubrika temporarily deployed to smoke-test September 21, 2022 18:41 Inactive
@aubrika aubrika temporarily deployed to smoke-test September 21, 2022 18:43 Inactive
@aubrika aubrika self-assigned this Sep 21, 2022
@aubrika aubrika changed the title wip: implement TransactionView and pcli v tx wip: implement TransactionView Sep 21, 2022
@aubrika aubrika marked this pull request as draft September 21, 2022 22:18
@aubrika aubrika marked this pull request as ready for review September 22, 2022 15:04
@aubrika aubrika marked this pull request as draft September 22, 2022 15:06
@aubrika aubrika temporarily deployed to smoke-test September 22, 2022 15:16 Inactive
@aubrika aubrika temporarily deployed to smoke-test September 22, 2022 15:49 Inactive
@aubrika aubrika force-pushed the transaction-view branch 3 times, most recently from 15130f4 to 01f888f Compare September 27, 2022 19:30
@aubrika aubrika temporarily deployed to smoke-test September 27, 2022 19:30 Inactive
@redshiftzero @aubrika
Implement transaction view structs & decryption logic
b8af99e 
Co-authored by: aubreka <aubrey@penumbra.zone>
@aubrika aubrika temporarily deployed to smoke-test September 27, 2022 19:52 Inactive
@aubrika aubrika marked this pull request as ready for review September 28, 2022 03:27
@aubrika aubrika changed the title wip: implement TransactionView implement TransactionView Sep 28, 2022
hdevalence
hdevalence reviewed Sep 28, 2022
View reviewed changes
transaction/src/action/output.rs
let payload_key = txp
.payload_keys
.get(&note_commitment)
.ok_or_else(|| anyhow::anyhow!("corresponding payload key not found"))?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error message is pretty opaque, and I think it won't give much information if it's encountered. Perhaps

perspective is missing payload key for output note commitment {}

would be better, since it identifies exactly what's missing and in what context.

hdevalence
hdevalence reviewed Sep 28, 2022
View reviewed changes
transaction/src/lib.rs Outdated
@@ -8,10 +8,12 @@ mod witness_data;

pub mod action;
pub mod plan;
pub mod transaction_view;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is already in the transaction crate, it'd be better to call the module view, so that instead of penumbra_transaction::transaction_view, we have penumbra_transaction::view.

@aubrika
Copy link
Contributor

aubrika commented Sep 29, 2022

Following yesterday's design discussion, this work will be refactored to include an ActionView associated with each Action, with the inner type of the ActionViews of transparent/currently-unencrypted Actions simply being the associated Actiontype. This will allow forpcli v txto handle a Vec ofActionView` rather than special-casing those transaction actions with encrypted contents.

@aubrika aubrika temporarily deployed to smoke-test September 29, 2022 18:16 Inactive
hdevalence
hdevalence reviewed Sep 30, 2022
View reviewed changes
proto/proto/penumbra/core/transaction/v1alpha1/transaction.proto Outdated
Comment on lines 98 to 99
Spend spend = 1;
Output output = 2;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These ones (and the Swap, SwapClaim below) are the four that need replacing with view-specific protos, right?

hdevalence
hdevalence reviewed Sep 30, 2022
View reviewed changes
transaction/src/action/delegate.rs Outdated
@@ -31,6 +33,10 @@ impl IsAction for Delegate {
fn balance_commitment(&self) -> penumbra_crypto::balance::Commitment {
self.balance().commit(Fr::zero())
}

fn decrypt_with_perspective(&self, txp: &TransactionPerspective) -> anyhow::Result<ActionView> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we have the design decision to have an "interpreted" ActionView, I think that this method would really be better named view_from_perspective, rather than decrypt -- and we won't need a Result, since viewing is infallible.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest we leave the signature for the moment & make an issue for next release to go over this work and make sure Results/Errors arising from composed function calls are handled in a way that's informative, because we can make view_from_perspective infallible, but there are component functions that may fail & I don't want to refactor around that thoughtlessly.

hdevalence
hdevalence reviewed Sep 30, 2022
View reviewed changes
transaction/src/view.rs Outdated Show resolved Hide resolved
@aubrika aubrika temporarily deployed to smoke-test September 30, 2022 23:32 Inactive
@aubrika aubrika merged commit fecd01b into main Oct 3, 2022
@aubrika aubrika deleted the transaction-view branch October 3, 2022 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hdevalence hdevalence hdevalence left review comments

@aubrika aubrika aubrika left review comments

Assignees

@aubrika aubrika

Labels
None yet
Projects
No open projects
Testnet #31: Autonoe
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@redshiftzero @aubrika @hdevalence