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

The BTC Confirm screen does not show the estimated FIAT value and estimated time #445

Open
onmax opened this issue Feb 14, 2022 · 5 comments

Comments

@onmax
Copy link
Member

onmax commented Feb 14, 2022

Problem

As you can see in the screenshot, currently, this screen is showing something different to what is supposed

Confirm

Solution

Before going to the code I want to make sure my solution is valid. Otherwise, we can discuss the right way to do it in this issue.

So right now, the interface we use for for this request is defined here: SignBtcTransactionRequestStandard. You can see an implementation of this interface I made for myself to understand it here

I propose changing this interface and update BitcoinTransactionInput adding the following keys: fiatAmount, fiatCurrency and estimatedDuration. Probably, estimated duration is not needed as we can calculate that in the keyguard itself, is this correct?

@sisou
Copy link
Member

sisou commented Feb 14, 2022

We require the Keyguard to NOT make any requests to any third party URL. So the Keyguard itself cannot determine the fiat equivalent of the amount, nor the estimated time until confirmation.

All data required for display need to be sent with the request.

You need to display three new pieces of information

  1. the fiat equivalent of the sent amount (output amount)
  2. the fiat equivalent of the fee (difference between inputs and outputs)
  3. the estimated duration until confirmation

We could either add these values to the inputs and outputs, but a BTC transaction can have multiple of each, so the Keyguard would have to sum them up and process them separately. The duration, meanwhile, would be a separate object property, as it doesn't belong to inputs nor outputs.

My suggestion is to add new fields to the BitcoinTransactionInfo, that are only for display, similar to how the swap has extra fields, only for display:

// Data needed for display
fiatCurrency: string,
fundingFiatRate: number,

These fields should probably also be the fiatCurrency, the fiatRate (because from the rate the Keyguard can calculate both the sent-amount fiat value, and also the fee fiat value), and as a third field the confirmationDuration, possibly in seconds.

@onmax
Copy link
Member Author

onmax commented Feb 15, 2022

I think I understood you correctly. I've added a temporary commit here, is that ok?

I noticed that in SignBtcTransactionRequestCheckout we are passing the fiatAmount as an argument directly. Passing the fiatRate as you mentioned is cleaner, so I've marked it as deprecated and if it's ok with you I can change SignBtcTransactionRequestCheckout after this issue.

Just one more question I would like to check this, the confirmationDuration field will be in seconds, so I have to move the hand clock:

  • to the left if duration <= 15 x 60 = 900 (less than 15m).
  • vertical if duration > 15 x 60 = 900 && duration <= 60 x 60 x 4= 14400 (between 15m and 4h)
  • to the right if duration > 60 x 60 x 4= 14400 (more than 4h)

@sisou
Copy link
Member

sisou commented Feb 15, 2022

I think your commit is going in the right direction, yes.

Regarding the gauge that displays the transaction "speed": The further to the right the hand points, the faster the tx. So when duration <= 15 x 60 = 900 (less than 15m), then the speed is high, so hand needs to point right, and vice-versa.

@onmax
Copy link
Member Author

onmax commented Feb 15, 2022

Got it, thanks. Therefore I am going to work on this

@danimoh
Copy link
Member

danimoh commented Feb 16, 2022

I'm not sure whether this is a good change.
The Keyguard has to display the user what he's actually signing, which is the BTC amounts and not the fiat amounts.
Therefore, the btc fee definitely needs to be displayed somewhere.
I suppose this would be moved to the tooltip behind the fiat fee?
This makes the actual important information pretty hidden, while the fiat amounts are prominent which can be arbitrary as the Keyguard can't really validate these.
So imo, this is not really an upgrade.

Also, not using external APIs has not historically been a strict requirement in the past.
The Keyguard already uses the coingecko api, see FiatApi.js which could also be used here for displaying fiat amounts.
The fetched values are of course properly parsed / sanitized.
For consistency to the other sign tx apis, however, the fiat rate should be passed from the caller as you guys discussed.
In both cases, the rate being handed in from the caller or being requested from coingecko, the trust assumption is similar.
It's an external value we have to trust.
The difference is, that requesting the rate from coingecko sends the user's IP to their servers which is not ideal.

But based on this, for extra paranoia, we could think about checking the passed rate against coingecko as some form of validating the fiat rate.
This could be on active user request only by him clicking on an update/validate button, e.g. in the tooltip, that states that rates will be fetched from coingecko, to only fetch the data with the user's consent.
That's a nice to have, but we could think about it.
I think it makes sense to avoid the Keyguard displaying arbitrary untrusted data.
If the two rates deviate too much, a warning could be shown.

We could also proxy the coingecko api through one of our servers, in that case we can fetch the data without consent.
Would be a good idea anyways.

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

No branches or pull requests

3 participants