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

fix: issue with fee UI react rendering loop #65

Closed
wants to merge 1 commit into from

Conversation

jrwbabylonlab
Copy link
Collaborator

@jrwbabylonlab jrwbabylonlab commented Aug 12, 2024

The bug happens when you stake amount + fee > funds avaiable:
image

  1. "Loading transaction fee amount..." forever
  2. Keep showing popup like below (periodically which interrupted the user interaction with dApp)
image 3. preview modal was shown when staking form data isn't valid. in this case, the fee is 0. and 4. No clear message about funds not enough to cover fees

This is a high-risk change, but it’s the least invasive approach I could think of without breaking other functionality.

What’s wrong with the current implementation?

The three components are currently tangled together:

•	staking.tsx
•	stakingAmount.tsx
•	stakingFee.tsx

The stakingFeeSat is derived in staking.tsx, but it’s passed as a prop to stakingFee.tsx and is affected by selectedFeeRate, which is also set within stakingFee.tsx. This results in two different places determining the fee values.

To make it more complex, stakingAmount.tsx handles input validation and decides the value that will determine UTXOs, which in turn affects the fees. As a result, these three components create a very complex interdependent React data flow through props and state.

I didn’t attempt to fix these interdependencies in this PR because doing so would require much larger changes. The ideal state I envision is:

•	All fee logic should reside only within stakingFee.tsx.
•	stakingFee.tsx should be a child component of stakingAmount.tsx.
•	The Preview modal component should obtain the value directly from stakingAmount.tsx using hooks. We don’t need onStakingAmountSatChange; instead, we need a getValidStakingAmount function when rendering the preview modal.

As a result of this PR, you’ll notice that the StakingFee component re-renders with each amount input. This isn’t avoidable unless we untangle the three components mentioned above. While it’s not ideal, it’s something we can live with for now until we refactor.

@jrwbabylonlab jrwbabylonlab changed the title fix: issue with fee UI infinite react rendering loop fix: issue with fee UI react rendering loop Aug 12, 2024
@jrwbabylonlab jrwbabylonlab force-pushed the fix-fee-infinite-react-loop branch from 4b6fa4b to 222915f Compare August 12, 2024 06:53
@@ -46,6 +46,11 @@ export const StakingFee: React.FC<StakingFeeProps> = ({
}
};

// If staking fee has not been calculated, return null
Copy link
Member

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 instead of what we had before? Seems that we have a different behavior for mempool fee rates

@jrwbabylonlab
Copy link
Collaborator Author

Going to close this PR as it introduce too much risk for the up coming release.

@jrwbabylonlab jrwbabylonlab deleted the fix-fee-infinite-react-loop branch October 2, 2024 03:40
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.

2 participants