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/1442 - Extension: Updates to MASP Tx Details #1506

Merged
merged 3 commits into from
Jan 27, 2025
Merged

feat/1442 - Extension: Updates to MASP Tx Details #1506

merged 3 commits into from
Jan 27, 2025

Conversation

jurevans
Copy link
Collaborator

@jurevans jurevans commented Jan 13, 2025 •
edited by mateuszjasiuk
Loading

Resolves #1442

  • Display shielded section hash correctly
  • Detect MASP transfer type correctly
    • Shielding
    • Unshielding
    • Shielded

@jurevans jurevans self-assigned this Jan 13, 2025
@jurevans jurevans added this to the Phase 3 milestone Jan 13, 2025
@jurevans jurevans changed the title WIP: feat/1442 = Extension: Updates to MASP Tx Details WIP: feat/1442 - Extension: Updates to MASP Tx Details Jan 13, 2025
@jurevans jurevans force-pushed the feat/1442-shielded-details branch 5 times, most recently from 610cc7e to 79ce09b Compare January 16, 2025 10:37
@jurevans jurevans force-pushed the feat/1442-shielded-details branch from 79ce09b to 73219ac Compare January 21, 2025 10:30
@mateuszjasiuk mateuszjasiuk force-pushed the feat/1442-shielded-details branch from 3cb0ddb to d4ed65b Compare January 24, 2025 15:27
@mateuszjasiuk mateuszjasiuk force-pushed the feat/1442-shielded-details branch from d4ed65b to d9cc843 Compare January 27, 2025 10:52
@mateuszjasiuk mateuszjasiuk marked this pull request as ready for review January 27, 2025 10:52
@mateuszjasiuk mateuszjasiuk changed the title WIP: feat/1442 - Extension: Updates to MASP Tx Details feat/1442 - Extension: Updates to MASP Tx Details Jan 27, 2025
@mateuszjasiuk mateuszjasiuk requested review from mateuszjasiuk, euharrison and pedrorezende and removed request for mateuszjasiuk January 27, 2025 10:53
mateuszjasiuk
mateuszjasiuk approved these changes Jan 27, 2025
View reviewed changes
Copy link
Collaborator

@mateuszjasiuk mateuszjasiuk left a comment

Choose a reason for hiding this comment

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

LGTM! :)

jurevans
jurevans commented Jan 27, 2025
View reviewed changes
apps/extension/src/Approvals/Commitment.tsx Outdated
import { TransactionCard } from "./TransactionCard";

type CommitmentProps = {
commitment: CommitmentDetailProps;
wrapperFeePayer?: string;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Question: Wouldn't wrapperFeePayer always be defined here? Even if we don't provide it as an initial Tx argument, I think this will always be set to an address by the time it is deserialized. If that's the case, this wouldn't need to be optional, but I'm not completely sure

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I think it's always there. I can typecast :D

jurevans
jurevans commented Jan 27, 2025
View reviewed changes
Copy link
Collaborator Author

@jurevans jurevans left a comment

Choose a reason for hiding this comment

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

LGTM! I can't actually approve this since I opened it initially. Just left one very minor comment, but feel free to merge!

jurevans
jurevans commented Jan 27, 2025
View reviewed changes
apps/extension/src/Approvals/Commitment.tsx Outdated

const renderContent = (
tx: CommitmentDetailProps,
wrapperFeePayer?: string
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this wrapperFeePayer doesn't need to be optional

@mateuszjasiuk mateuszjasiuk force-pushed the feat/1442-shielded-details branch from e550e89 to 929c570 Compare January 27, 2025 11:31
@mateuszjasiuk mateuszjasiuk merged commit 4ee780f into main Jan 27, 2025
7 checks passed
@mateuszjasiuk mateuszjasiuk deleted the feat/1442-shielded-details branch January 27, 2025 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mateuszjasiuk mateuszjasiuk mateuszjasiuk approved these changes

@euharrison euharrison Awaiting requested review from euharrison

@pedrorezende pedrorezende Awaiting requested review from pedrorezende

Assignees

@jurevans jurevans

Labels
App: Extension bug Something isn't working Lib: SDK masp
Projects
None yet
Milestone
Phase 3
Development

Successfully merging this pull request may close these issues.

Display MASP TX details in the confirmation popup
2 participants
@jurevans @mateuszjasiuk