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.

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

frontend: change transaction info for send-to-self #3035

Merged

Conversation

thisconnect
Copy link
Collaborator

Until now a send-to-self transaction looked more like an outgoing transaction with the amount sent.

Changed to only show the fee paid as outgoing and the amount of coin as conversion amount.

@@ -221,8 +221,8 @@ func (handlers *Handlers) getTxInfoJSON(txInfo *accounts.TransactionData, detail
Note: handlers.account.TxNote(txInfo.InternalID),
}

txInfoJSON.Fee = feeString
if detail {
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 slightly prefer if we could just pass all that data anyways.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

interestingly TypeScript already expected that there is fee, also it thinks there is gas and nonce. I am planing to open another PR to fix the types and make gas/nonce only be in /transaction?id= endpoint and optional.

fee: IAmount;
feeRatePerKb: IAmount;
gas: number;
nonce: number | null;

frontends/web/src/components/transactions/transaction.tsx Outdated Show resolved Hide resolved
frontends/web/src/components/transactions/transaction.tsx Outdated Show resolved Hide resolved
@thisconnect
Copy link
Collaborator Author

rebased

@thisconnect
Copy link
Collaborator Author

Screenshot_2024-11-12_at_08 17 29

@thisconnect thisconnect force-pushed the frontend-improve-sendtoself branch 2 times, most recently from e39c0dc to 9e5dfe0 Compare November 20, 2024 10:55
@thisconnect thisconnect marked this pull request as ready for review November 20, 2024 12:10
Copy link
Collaborator

@shonsirsha shonsirsha left a comment

Choose a reason for hiding this comment

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

FE tested LGTM:

p_P p_PA

@Tomasvrba
Copy link
Contributor

Tomasvrba commented Nov 22, 2024

@thisconnect nack from me, personally showing the fee as the bigger number (and therefore the main number always being negative) seems really counterintuitive, I would switch them around, the send amount with the "send to self" icon as the main number and the fee as the smaller number below (ideally with the label "fee")

Always having these long string fee amounts as the main tx amount doesn't look very good:
Screenshot From 2024-11-22 11-29-19

@thisconnect
Copy link
Collaborator Author

personally showing the fee as the bigger number (and therefore the main number always being negative) seems really counterintuitive, I would switch them around, the send amount with the "send to self" icon as the main number and the fee as the smaller number below (ideally with the label "fee")

Thanks for the input, the main reason to show the fee as negative "bigger number" is so it shows what was spent for this transaction.

Always having these long string fee amounts as the main tx amount doesn't look very good

In ETH this indeed shows very long numbers due to 18 digits.

I will discuss both points internally...

@thisconnect
Copy link
Collaborator Author

@Tomasvrba did you test with tokens as well?

@Tomasvrba
Copy link
Contributor

@Tomasvrba did you test with tokens as well?

Yes, same issue from my perspective, since it's a token "account" I would expect the main tx number be the token transferred in the tx, not the eth-denominated fee

Screenshot From 2024-11-22 11-58-18

@thisconnect
Copy link
Collaborator Author

rebased

@thisconnect
Copy link
Collaborator Author

showing the fee as the bigger number (and therefore the main number always being negative) seems really counterintuitive, I would switch them around, the send amount with the "send to self" icon as the main number and the fee as the smaller number below (ideally with the label "fee")

As discussed internally:

For send-to-self transactions we keep showing the fee as the bigger number so that the user see what they spent to do so.

In ERC20 tokens the fee is in ETH that is how Ethereum works, but

…there seems to be no good use-case to do send-to-self on account based coins/tokens. If you find out how contract interaction would look like in the app where the user initiates some kind of withdrawal which results in a deposit to the user's address please let me know.

@thisconnect
Copy link
Collaborator Author

rebased, cc @Beerosagos PTAL at the backend change.

@@ -221,8 +221,8 @@ func (handlers *Handlers) getTxInfoJSON(txInfo *accounts.TransactionData, detail
Note: handlers.account.TxNote(txInfo.InternalID),
}

txInfoJSON.Fee = feeString
Copy link
Collaborator

Choose a reason for hiding this comment

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

This works, but it would be better as Fee: feeString, inside the txInfoJSON object creation above

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@thisconnect thisconnect force-pushed the frontend-improve-sendtoself branch 2 times, most recently from 8aa34f6 to c635b55 Compare November 26, 2024 09:39
Copy link
Collaborator

@Beerosagos Beerosagos left a comment

Choose a reason for hiding this comment

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

backend LGTM

@thisconnect
Copy link
Collaborator Author

rebased

Until now a send-to-self transaction looked more like an outgoing
transaction with the amount sent.

Changed to only show the fee paid as outgoing and the amount of
coin as conversion amount.
@thisconnect thisconnect merged commit ebe632f into BitBoxSwiss:master Nov 26, 2024
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants