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

feat: add summary to TransactionInfo #5102

Merged
merged 5 commits into from
Feb 21, 2025
Merged

feat: add summary to TransactionInfo #5102

merged 5 commits into from
Feb 21, 2025

Conversation

VanishMax
Copy link
Contributor

Adds a summary field of type TransactionSummary to TransactionInfo proto.

Mostly needed for Web because Rust ecosystem can simply do tx_info.view.summary() but Web would have to re-implement this feature in TypeScript

@VanishMax VanishMax self-assigned this Feb 21, 2025
@VanishMax VanishMax mentioned this pull request Feb 21, 2025
Open
1 task
TalDerei
TalDerei reviewed Feb 21, 2025
View reviewed changes
Copy link
Collaborator

@TalDerei TalDerei left a comment

Choose a reason for hiding this comment

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

penumbra-zone/web#2000 (comment) – will defer to @erwanor on where the transaction summary should live

cronokirby
cronokirby approved these changes Feb 21, 2025
View reviewed changes
Copy link
Contributor

@cronokirby cronokirby left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This makes sense ; putting this in the view service avoids the need to duplicate the code, since we can re-use the Rust method in the WASM underlying the view service. I think adding this to the info is better than creating a separate RPC.

@conorsch conorsch merged commit 9e1c21d into main Feb 21, 2025
9 checks passed
@conorsch conorsch deleted the feat/add-summary-to-tx-info branch February 21, 2025 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TalDerei TalDerei TalDerei left review comments

@cronokirby cronokirby cronokirby approved these changes

@erwanor erwanor Awaiting requested review from erwanor

Assignees

@VanishMax VanishMax

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@VanishMax @cronokirby @TalDerei @conorsch