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

fix: incorrect ui with psbt listing tx #4441

Merged
merged 1 commit into from
Oct 27, 2023
Merged

fix: incorrect ui with psbt listing tx #4441

merged 1 commit into from
Oct 27, 2023

Conversation

fbwoolf
Copy link
Contributor

@fbwoolf fbwoolf commented Oct 26, 2023 •
edited by github-actions bot
Loading

Try out this version of Leather — download extension builds.

This PR fixes some reported UI bugs with PSBT listing txs...

No longer showing an inscription as being received if the address is transferring it...
Screen Shot 2023-10-26 at 11 19 13 AM

If there is no fee, show 0...
Screen Shot 2023-10-26 at 11 19 50 AM

@fbwoolf fbwoolf linked an issue Oct 26, 2023 that may be closed by this pull request
Fix listing PSBT issues on Gamma #4428
Closed
@markmhendrickson markmhendrickson self-requested a review October 27, 2023 07:28
@markmhendrickson
Copy link
Collaborator

Thanks! I'll help test this build with buy and sell flows

kyranjamie
kyranjamie reviewed Oct 27, 2023
View reviewed changes
Copy link
Collaborator

@kyranjamie kyranjamie left a comment

Choose a reason for hiding this comment

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

Should we be adding guidance that, there is a fee, just that this user isn't paying it?

...psbt-signer/components/psbt-inputs-outputs-totals/components/psbt-address-receive-totals.tsx Outdated
Comment on lines 35 to 39
subValue={i18nFormatCurrency(calculateBitcoinFiatValue(addressNativeSegwitTotal)).replace(
'-',
''
)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we doing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We subtract inputs from outputs to determine transfer vs receive, but we don't need to show the value displayed as negative if receiving.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can make a remove minus sign function than repeating?

@markmhendrickson
Copy link
Collaborator

@mica000 @fabric-8 wdyt of removing this intro paragraph from the PSBT prompt as part of this PR? To me, it's a bit redundant since the screen already implies the need to review details and approve. It causes extra mental overload and pushes the main data down the view and some below the fold.

Screenshot 2023-10-27 at 13 24 39

@markmhendrickson
Copy link
Collaborator

This may be best to break out into a separate issue for tackling later, but I'm seeing slight styling differences between the first and second bullet point here.

Is this a PSBT-error-specific thing or a generic-error-modal thing?

Screenshot 2023-10-27 at 13 25 16

@mica000
Copy link

mica000 commented Oct 27, 2023

This may be best to break out into a separate issue for tackling later, but I'm seeing slight styling differences between the first and second bullet point here.

Is this a PSBT-error-specific thing or a generic-error-modal thing?

Screenshot 2023-10-27 at 13 25 16

Would also add that instead of a link, the blose CTA should be a solid button

@markmhendrickson
Copy link
Collaborator

This PR's main purpose of fixing the listing UX on Gamma works in my testing! 🥳

The summary now shows accurately that you'll be giving up an inscription in exchange for the desired BTC amount, and that as a seller, you'll pay no network fees.

Screenshot 2023-10-27 at 14 05 35

@markmhendrickson
Copy link
Collaborator

markmhendrickson commented Oct 27, 2023 •
edited
Loading

In the same spirit of removing redundancy from this modal (which we can break out into a separate issue as well), @mica000 wdyt of removing this "Transaction" header as well, and moving the security pill (e.g. "Certain") up to the top right of the dialog?

Screenshot 2023-10-27 at 14 09 51

@fbwoolf
Copy link
Contributor Author

fbwoolf commented Oct 27, 2023

Is this a PSBT-error-specific thing or a generic-error-modal thing?

This is using our GenericError component.

@fbwoolf
Copy link
Contributor Author

fbwoolf commented Oct 27, 2023

Should we be adding guidance that, there is a fee, just that this user isn't paying it?

Gamma does let the user know in their UI before our popup is launched, but not sure that is the case for all apps.

@markmhendrickson
Copy link
Collaborator

Should we be adding guidance that, there is a fee, just that this user isn't paying it?

Is this data captured in the PSBT itself, and would it be shown as something like "Somebody else will pay X in fees"? I'm not really sure the signing / non-paying user needs to know this, though could be convinced it's helpful I guess.

@fbwoolf
Copy link
Contributor Author

fbwoolf commented Oct 27, 2023

Is this data captured in the PSBT itself, and would it be shown as something like "Somebody else will pay X in fees"? I'm not really sure the signing / non-paying user needs to know this, though could be convinced it's helpful I guess.

It seems like since we are showing the fee as 0, that is enough on our end? The PSBT doesn't really have this information except that the inputs/outputs diff is 0.

kyranjamie
kyranjamie approved these changes Oct 27, 2023
View reviewed changes
...psbt-signer/components/psbt-inputs-outputs-totals/components/psbt-address-receive-totals.tsx Outdated
Comment on lines 35 to 39
subValue={i18nFormatCurrency(calculateBitcoinFiatValue(addressNativeSegwitTotal)).replace(
'-',
''
)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can make a remove minus sign function than repeating?

@fbwoolf fbwoolf added this pull request to the merge queue Oct 27, 2023
Merged via the queue into dev with commit e83db28 Oct 27, 2023
26 checks passed
@fbwoolf fbwoolf deleted the fix/psbt-listing branch October 27, 2023 15:22
@fbwoolf
Copy link
Contributor Author

fbwoolf commented Oct 27, 2023

Merged this fix, @markmhendrickson can you create a new issue for UI changes you want separately?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alter-eggo alter-eggo alter-eggo approved these changes

@kyranjamie kyranjamie kyranjamie approved these changes

@pete-watters pete-watters Awaiting requested review from pete-watters

@markmhendrickson markmhendrickson Awaiting requested review from markmhendrickson

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix listing PSBT issues on Gamma
5 participants
@fbwoolf @markmhendrickson @mica000 @kyranjamie @alter-eggo