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

Deposit all doesn't work when the fee rate is updated #216

Closed
Keith-CY opened this issue Jul 7, 2023 · 22 comments
Closed

Deposit all doesn't work when the fee rate is updated #216

Keith-CY opened this issue Jul 7, 2023 · 22 comments
Assignees
Labels
bug Something isn't working

Comments

@Keith-CY
Copy link
Member

Keith-CY commented Jul 7, 2023

Screen.Recording.2023-07-07.at.14.35.16.mov

The fee turns into 0 when fee rate is updated

@Keith-CY Keith-CY added the bug Something isn't working label Jul 7, 2023
@Keith-CY Keith-CY added this to Neuron Jul 7, 2023
@Danie0918 Danie0918 moved this to 🏗 In Progress in Neuron Jul 7, 2023
@Danie0918 Danie0918 assigned yanguoyu and unassigned Danie0918 Jul 7, 2023
@yanguoyu
Copy link

This issue may occur in the current interaction mode. Currently, when the deposit value change, Neuron will check whether the deposit value is bigger than the max deposit value, if so, the deposit value will change to the max deposit value, or else generate a transaction with the current deposit value.

When this issue will occur:

At first, we enter a bigger value than the max deposit value, it will change to the max deposit value. But if the suggestFeeRate becomes less, the max deposit value will increase. Then generating a transaction with the last max deposit value will fail because the remaining ckb can not hold a cell.
It will generate success until the suggestFeeRate becomes bigger than the first suggestFeeRate.

I propose to add a max button to let the user set deposit all ckb, and when the user selects deposit all, the input will be disabled and the input value set as the max deposit value.
On the other hand, I propose the Slide component below the deposit input is used for changing the deposit percent but not the deposit value, then selecting 100% percent is the same as selecting deposit all.

@Danie0918 What do you think?

@Danie0918
Copy link
Contributor

Danie0918 commented Jul 11, 2023

@Danie0918 What do you think?

The main problem we have here is that the maximum deposit value no longer satisfies the current transaction conditions when the suggestedFeeRate changes. For this problem, you proposed point 2 optimization which I think can work, suggesting to optimize the slider component for changing the deposit percentage, where the amount and fee of the input box changes dynamically, and the summed value is the set deposit percentage. The button for the maximum value is not necessary for the current interaction and can be left out.

@yanguoyu
Copy link

@Danie0918 What do you think?

The main problem we have here is that the maximum deposit value no longer satisfies the current transaction conditions when the suggestedFeeRate changes. For this problem, you proposed point 2 optimization which I think can work, suggesting to optimize the sliding component for changing the deposit percentage, where the amount and fee of the input box changes dynamically and sums up to the set deposit percentage. The button for the maximum value is not necessary for the current interaction and can be left out.

Then we will remove the logic that set the deposit to the max deposit value when the user enters over than the max deposit value, if they want to deposit all, they can slide the slider to 100%.
Is that what you mean?

@Danie0918
Copy link
Contributor

Then we will remove the logic that set the deposit to the max deposit value when the user enters over than the max deposit value, if they want to deposit all, they can slide the slider to 100%. Is that what you mean?

Yes, the suggestedFeeRate is out of the user's control, when the user chooses to drag to the maximum, the user's expectation is to deposit the maximum amount, so when the suggestedFeeRate changes the maximum entered should change accordingly to ensure that the transaction takes place.

@yanguoyu
Copy link

yanguoyu commented Jul 11, 2023

Then we will remove the logic that set the deposit to the max deposit value when the user enters over than the max deposit value, if they want to deposit all, they can slide the slider to 100%. Is that what you mean?

Yes, the suggestedFeeRate is out of the user's control, when the user chooses to drag to the maximum, the user's expectation is to deposit the maximum amount, so when the suggestedFeeRate changes the maximum entered should change accordingly to ensure that the transaction takes place.

Cool, I will resolve this issue with these steps:

  1. If the deposit value is bigger than the max, warn the user that the amount is not enough, and they can deposit all by sliding the slider to 100%
  2. Change the slider value to percent, if the slider value is 100%, means deposit all, and currently when the suggestedFeeRate changes, the deposit value will also change.
  3. For other slider values, the deposit value will calculate by slider value * max, to keep the deposit value stable, the max deposit value here will calculate by stable suggestedFeeRate(2000).

@Keith-CY @Danie0918 @FrederLu

@Keith-CY
Copy link
Member Author

Keith-CY commented Jul 11, 2023

Then we will remove the logic that set the deposit to the max deposit value when the user enters over than the max deposit value, if they want to deposit all, they can slide the slider to 100%. Is that what you mean?

Yes, the suggestedFeeRate is out of the user's control, when the user chooses to drag to the maximum, the user's expectation is to deposit the maximum amount, so when the suggestedFeeRate changes the maximum entered should change accordingly to ensure that the transaction takes place.

Cool, I will resolve this issue with these steps:

  1. If the deposit value is bigger than the max, warn the user that the amount is not enough, and they can deposit all by sliding the slider to 100%
  2. Change the slider value to percent, if the slider value is 100%, means deposit all, and currently when the suggestedFeeRate changes, the deposit value will also change.
  3. For other slider values, the deposit value will calculate by slider value * max, to keep the deposit value stable, the max deposit value here will calculate by stable suggestedFeeRate(2000).

@Keith-CY @Danie0918 @FrederLu

It should work.

But IMO it can be resolved simply by fixing the fee rate when the amount is set. Isn't this solution enough for the case?

@yanguoyu
Copy link

It should work.

But IMO it can be resolved simply by fixing the fee rate when the amount is set. Is this solution not enough for the case?

Of cause, if the fee rate is fixed, there will be no problem. Currently, the fee rate will change every 30s.
Do you mean we can fix the fee rate?

@Keith-CY
Copy link
Member Author

Keith-CY commented Jul 11, 2023

It should work.
But IMO it can be resolved simply by fixing the fee rate when the amount is set. Is this solution not enough for the case?

Of cause, if the fee rate is fixed, there will be no problem. Currently, the fee rate will change every 30s. Do you mean we can fix the fee rate?

The solution comes from nervosnetwork/neuron#2760 and I find it's easy and clear

@yanguoyu
Copy link

It should work.
But IMO it can be resolved simply by fixing the fee rate when the amount is set. Is this solution not enough for the case?

Of cause, if the fee rate is fixed, there will be no problem. Currently, the fee rate will change every 30s. Do you mean we can fix the fee rate?

The solution comes from nervosnetwork/neuron#2760 and I find it's easy and clear

They are different problems, nervosnetwork/neuron#2760 fix the problem of when should we generate tx, but here is resolve how to generate tx.

@Keith-CY
Copy link
Member Author

Keith-CY commented Jul 11, 2023

It should work.
But IMO it can be resolved simply by fixing the fee rate when the amount is set. Is this solution not enough for the case?

Of cause, if the fee rate is fixed, there will be no problem. Currently, the fee rate will change every 30s. Do you mean we can fix the fee rate?

The solution comes from nervosnetwork/neuron#2760 and I find it's easy and clear

They are different problems, nervosnetwork/neuron#2760 fix the problem of when should we generate tx, but here is resolve how to generate tx.

Correct me if I'm wrong.

The problem is introduced by regenerating a transaction with a user-specified amount and a chain-specified fee rate by the following steps:

  1. the max amount is specified by user, and a transaction is generated
  2. a smaller fee rate is specified by the chain, and a transaction is failed to generate because less than 61 CKB will be left due to Δfee_rate

If step 2 is prohibited, there won't Δfee_rate or less than 61 CKB left. In this case, the transaction to send is only generated when the amount is updated manually, with the suggested fee rate at that time

@Keith-CY
Copy link
Member Author

Keith-CY commented Jul 11, 2023

It should work.
But IMO it can be resolved simply by fixing the fee rate when the amount is set. Is this solution not enough for the case?

Of cause, if the fee rate is fixed, there will be no problem. Currently, the fee rate will change every 30s. Do you mean we can fix the fee rate?

The solution comes from nervosnetwork/neuron#2760 and I find it's easy and clear

They are different problems, nervosnetwork/neuron#2760 fix the problem of when should we generate tx, but here is resolve how to generate tx.

Correct me if I'm wrong.

The problem is introduced by regenerating a transaction with a user-specified amount and a chain-specified fee rate by the following steps:

  1. the max amount is specified by user, and a transaction is generated
  2. a smaller fee rate is specified by the chain, and a transaction is failed to generate because less than 61 CKB will be left due to Δfee_rate

If step 2 is prohibited, there won't Δfee_rate or less than 61 CKB left. In this case, the transaction to send is only generated when the amount is updated manually, with the suggested fee rate at that time

Simply to say, the suggested fee rate is cached when the dialog is open and a transaction has been generated, until the amount is updated

@yanguoyu
Copy link

It should work.
But IMO it can be resolved simply by fixing the fee rate when the amount is set. Is this solution not enough for the case?

Of cause, if the fee rate is fixed, there will be no problem. Currently, the fee rate will change every 30s. Do you mean we can fix the fee rate?

The solution comes from nervosnetwork/neuron#2760 and I find it's easy and clear

They are different problems, nervosnetwork/neuron#2760 fix the problem of when should we generate tx, but here is resolve how to generate tx.

Correct me if I'm wrong.
The problem is introduced by regenerating a transaction with a user-specified amount and a chain-specified fee rate by the following steps:

  1. the max amount is specified by user, and a transaction is generated
  2. a smaller fee rate is specified by the chain, and a transaction is failed to generate because less than 61 CKB will be left due to Δfee_rate

If step 2 is prohibited, there won't Δfee_rate or less than 61 CKB left. In this case, the transaction to send is only generated when the amount is updated manually, with the suggested fee rate at that time

Simply to say, the suggested fee rate is cached when the dialog is open and a transaction has been generated, until the amount is updated

Of cause, if the fee rate is fixed until users change the value, it also can resolve this issue. But it will change the original logic, is it reasonable? I'm not sure.

@Danie0918 What do you think?

@Keith-CY
Copy link
Member Author

It should work.
But IMO it can be resolved simply by fixing the fee rate when the amount is set. Is this solution not enough for the case?

Of cause, if the fee rate is fixed, there will be no problem. Currently, the fee rate will change every 30s. Do you mean we can fix the fee rate?

The solution comes from nervosnetwork/neuron#2760 and I find it's easy and clear

They are different problems, nervosnetwork/neuron#2760 fix the problem of when should we generate tx, but here is resolve how to generate tx.

Correct me if I'm wrong.
The problem is introduced by regenerating a transaction with a user-specified amount and a chain-specified fee rate by the following steps:

  1. the max amount is specified by user, and a transaction is generated
  2. a smaller fee rate is specified by the chain, and a transaction is failed to generate because less than 61 CKB will be left due to Δfee_rate

If step 2 is prohibited, there won't Δfee_rate or less than 61 CKB left. In this case, the transaction to send is only generated when the amount is updated manually, with the suggested fee rate at that time

Simply to say, the suggested fee rate is cached when the dialog is open and a transaction has been generated, until the amount is updated

Of cause, if the fee rate is fixed until users change the value, it also can resolve this issue. But it will change the original logic, is it reasonable? I'm not sure.

@Danie0918 What do you think?

IMO, the amount to deposit is confusing if the fee rate is dynamic because the amount is dynamic too, the only value static is the total value to send, including dynamic amount and dynamic fee, so the UX should change accordingly
cc @Danie0918

@yanguoyu
Copy link

On the other hand, it also happens on the send page, if the users select max to send, the max sends ckb will also change every 30s because the fee rate will change every 30s.

@Keith-CY
Copy link
Member Author

On the other hand, it also happens on the send page, if the users select max to send, the max sends ckb will also change every 30s because the fee rate will change every 30s.

Personal idea: a checkbox for send all is 100% clearer than the slider

@Danie0918
Copy link
Contributor

Danie0918 commented Jul 11, 2023

Can you describe the rules and requirements here about suggestedFeeRate? I'm not sure that a fixed rate is consistent with the original design intent here. @yanguoyu

In my opinion suggestedFeeRate should be dynamic to help close deals, then using a fixed rate here doesn't meet expectations.

@yanguoyu
Copy link

Can you describe the rules and requirements here about suggestedFeeRate? I'm not sure that a fixed rate is consistent with the original design intent here. @yanguoyu

In my opinion suggestedFeeRate should be dynamic to help close deals, then using a fixed rate here doesn't meet expectations.

suggestedFeeRate gets from the blockchain by rpc. It is used for users to let the tx broadcast easily, if the fee rate is small, the tx may broadcast slowly. Currently, the suggestedFeeRate will get from the rpc every 30s.

@Danie0918
Copy link
Contributor

Danie0918 commented Jul 11, 2023

Can you describe the rules and requirements here about suggestedFeeRate? I'm not sure that a fixed rate is consistent with the original design intent here. @yanguoyu
In my opinion suggestedFeeRate should be dynamic to help close deals, then using a fixed rate here doesn't meet expectations.

suggestedFeeRate gets from the blockchain by rpc. It is used for users to let the tx broadcast easily, if the fee rate is small, the tx may broadcast slowly. Currently, the suggestedFeeRate will get from the rpc every 30s.

In that case, I'd recommend still using changing the logic here in Nervos DAO and Send to change dynamically when the maximum value is selected.

@yanguoyu
Copy link

So, what should we finally do? @Danie0918 @Keith-CY

Add a max button to set max, and keep the dynamic fee when the users select deposit max?

@Danie0918
Copy link
Contributor

So, what should we finally do? @Danie0918 @Keith-CY

Add a max button to set max, and keep the dynamic fee when the users select deposit max?

It's better to adjust the logic of the slider, the current interaction already meets the needs of the maximum value. Dynamically change the amount when the slider reaches the maximum. @Keith-CY

@Keith-CY
Copy link
Member Author

So, what should we finally do? @Danie0918 @Keith-CY
Add a max button to set max, and keep the dynamic fee when the users select deposit max?

It's better to adjust the logic of the slider, the current interaction already meets the needs of the maximum value. Dynamically change the amount when the slider reaches the maximum. @Keith-CY

LGTM

@yanguoyu
Copy link

@Danie0918 Danie0918 moved this from 🏗 In Progress to 🔎 Code Review in Neuron Jul 16, 2023
@Danie0918 Danie0918 moved this from 🔎 Code Review to 👀 Testing in Neuron Jul 24, 2023
@silySuper silySuper self-assigned this Jul 24, 2023
@Danie0918 Danie0918 moved this from 👀 Testing to 🚩Pre Release in Neuron Jul 31, 2023
@yanguoyu yanguoyu assigned Danie0918 and unassigned yanguoyu and silySuper Aug 10, 2023
@Danie0918 Danie0918 moved this from 🚩Pre Release to ✅ Done in Neuron Jan 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

No branches or pull requests

5 participants