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

Feat/implement dynamic fee rate based on fee rate statistics #2599

Conversation

JeffreyMa597
Copy link
Contributor

@JeffreyMa597 JeffreyMa597 commented Mar 4, 2023

What problem does this PR solve?

As title.

An RPC to fetch fee_rate_statistics is added since CKB@v0.106.0, which returns mean fee rate and median fee rate of the specified block range.

With the historic data, Neuron could offer more precise fee rate recommendations for users.

PRD: Magickbase/neuron-public-issues#87
UI: https://www.figma.com/file/6XNoimRDbFTTNm016rbIdU/Magickbase?node-id=319%3A4539&t=oQrWTpg2GhW4obRJ-0

Ref: Magickbase/neuron-public-issues#95

Check List

1、The customer price and dropdown price can be switched.

image

2、The fee rate dropdown list is displayed with 3 options, and the suggested_fee_rate will be updated every 30 seconds.
1. Fast: fee rate is 2 * suggested_fee_rate - 1000
2. Standard: fee rate is suggested_fee_rate
3. Slow: fee rate is 1000

image

3、The fee in every single option will update when the fee rate is updated.
Of course, It's calculated by account and fee rate.
The standard one is selected by default and also when the selected value is not matched with any option.

image

4、The suggest_fee_rate will update every 30 seconds, also the fee is. The fee is based on the fee rate which we type in the text field. When the fee rate is set below 1000, there will be a warning tip instead of the suggest tip.

image

image

5、The fee in Nervos Dao, Customized assets and Assets account are all based on the suggest fee rate instead of using the medium value.

image

image

Test

e2e Test

Task

none

@yanguoyu
Copy link
Collaborator

yanguoyu commented Mar 9, 2023

Texts should be internationalization, and they should not use directly.

packages/neuron-ui/src/components/NFTSend/index.tsx Outdated Show resolved Hide resolved
packages/neuron-ui/src/components/NervosDAO/index.tsx Outdated Show resolved Hide resolved
packages/neuron-ui/src/components/NervosDAO/index.tsx Outdated Show resolved Hide resolved
packages/neuron-ui/src/components/PricePanel/index.tsx Outdated Show resolved Hide resolved
packages/neuron-ui/src/components/PricePanel/index.tsx Outdated Show resolved Hide resolved
@JeffreyMa597 JeffreyMa597 force-pushed the feat/support-dynamic-fee-rate-based-on-fee-rate-statistics branch 2 times, most recently from ee36633 to 622cbef Compare April 6, 2023 07:37
packages/neuron-wallet/src/controllers/api.ts Outdated Show resolved Hide resolved
packages/neuron-ui/src/containers/Navbar/index.tsx Outdated Show resolved Hide resolved
packages/neuron-ui/src/locales/en.json Outdated Show resolved Hide resolved
packages/neuron-ui/src/locales/en.json Outdated Show resolved Hide resolved
packages/neuron-ui/src/services/remote/remoteApiWrapper.ts Outdated Show resolved Hide resolved
packages/neuron-ui/src/services/remote/wallets.ts Outdated Show resolved Hide resolved
packages/neuron-ui/src/styles/color.scss Outdated Show resolved Hide resolved
@@ -5,7 +5,8 @@
* {
outline: none !important;
-webkit-tap-highlight-color: none !important;
cursor: default !important;
// why we need this property with value of '!important'
Copy link
Collaborator

Choose a reason for hiding this comment

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

It was required to act as a native app, and pointer is in web app exclusively.

packages/neuron-ui/src/utils/calculateFee.ts Show resolved Hide resolved
packages/neuron-ui/src/states/stateProvider/reducer.ts Outdated Show resolved Hide resolved
packages/neuron-ui/src/components/NervosDAO/index.tsx Outdated Show resolved Hide resolved
packages/neuron-ui/src/components/Send/hooks.ts Outdated Show resolved Hide resolved
packages/neuron-ui/src/components/Send/hooks.ts Outdated Show resolved Hide resolved
app: { isAllowedToFetchList = true },
app: {
isAllowedToFetchList = true,
countDown,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is countDown necessary to be global?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, it will be used to dispatch a request in another place.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The fee rate can be fetched on demand, then count down starts. And count down ends on leaving the page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1、The countdown is associated with the interface call. They are synchronized, and now the interface call is on the main page, so the countdown accordingly is there. And even if the countdown is not displayed, the interface needs to be called periodically on other pages to update fee_rate_statistics.
2、The display of the countdown is on the send page. I use a global variable to pass this value, not context.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why should the count down be updated when fee rate is not used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why should the count down be updated when fee rate is not used?

I mean the fee_rate will be used in some different places, and in order to keep fee_rate consistent and updated regularly everywhere, I put countdown and request together. And, certainly, I can use context to pass this value, but I think using redux may be more convenient.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe you can wrapper get the fee rate as a hook, like this, then every want get feeRate and countDown by call this hook

const useGetFeeRate = () => {
    const [countDown, setCountDown] = useState(30)
    const [feeRate, setFeeRate] = useState()
    useEffect(() => {
        const timeout = setTimeout(() => {
          setCountDown(v => v - 1)
        }, 1000)
        return () => clearTimeout(timeout)
    }, [])
    useEffect(() => {
     if (countDown === 0) {
        getFeeRateStats()
              .then(res => {
                const { mean, median } = res
                const suggested = res ? Math.max(1000, Number(mean), Number(median)) : 0
                setFeeRate({ ...res, suggestFeeRate: suggested })
              })
              .finaly {
                  setCountDown(30)
               }
        }
     }
    }, [countDown])
    return { feeRate, countDown }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe you can wrapper get the fee rate as a hook, like this, then every want get feeRate and countDown by call this hook

const useGetFeeRate = () => {
    const [countDown, setCountDown] = useState(30)
    const [feeRate, setFeeRate] = useState()
    useEffect(() => {
        const timeout = setTimeout(() => {
          setCountDown(v => v - 1)
        }, 1000)
        return () => clearTimeout(timeout)
    }, [])
    useEffect(() => {
     if (countDown === 0) {
        getFeeRateStats()
              .then(res => {
                const { mean, median } = res
                const suggested = res ? Math.max(1000, Number(mean), Number(median)) : 0
                setFeeRate({ ...res, suggestFeeRate: suggested })
              })
              .finaly {
                  setCountDown(30)
               }
        }
     }
    }, [countDown])
    return { feeRate, countDown }
}

Thx. I submitted a new version with a hook before I saw your response. Thanks for your help again. And please make a code review.

@JeffreyMa597 JeffreyMa597 force-pushed the feat/support-dynamic-fee-rate-based-on-fee-rate-statistics branch 9 times, most recently from 2217cd2 to 0446ab3 Compare April 13, 2023 04:59
@JeffreyMa597 JeffreyMa597 force-pushed the feat/support-dynamic-fee-rate-based-on-fee-rate-statistics branch from 0446ab3 to adbc342 Compare April 13, 2023 16:32
@yanguoyu
Copy link
Collaborator

This PR may need a tester to test and approve it.

@Keith-CY
Copy link
Collaborator

Keith-CY commented Apr 14, 2023

This PR may need a tester to test and approve it.

I’ve verified the functionality yesterday.

@Keith-CY Keith-CY merged commit 1cf76c1 into nervosnetwork:develop Apr 14, 2023
5 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.

None yet

4 participants