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.

Sign up for GitHub

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: fee issue utilizing fee hook #1679

Merged
merged 3 commits into from
Dec 2, 2024
Merged

fix: fee issue utilizing fee hook #1679

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cae5351
fix: fee issue utilizing fee hook
Nick-1979 Nov 30, 2024
6b58042
refactor: handle undefined case
Nick-1979 Nov 30, 2024
a24f3e8
fix: useEffects deps (#1680)
AMIRKHANEF Dec 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 4 additions & 10 deletions packages/extension-polkagate/src/fullscreen/accountDetails/unlock/Review.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@

import type { ApiPromise } from '@polkadot/api';
import type { SubmittableExtrinsic } from '@polkadot/api/types';
import type { Balance } from '@polkadot/types/interfaces';
import type { ISubmittableResult } from '@polkadot/types/types';
import type { BN } from '@polkadot/util';
import type { Lock } from '../../../hooks/useAccountLocks';
Expand All @@ -20,10 +19,10 @@ import { faLockOpen, faUserAstronaut } from '@fortawesome/free-solid-svg-icons';
import { Container, Grid, useTheme } from '@mui/material';
import React, { useCallback, useEffect, useMemo, useState } from 'react';

import { BN_ONE, isBn } from '@polkadot/util';
import { isBn } from '@polkadot/util';

import { AccountHolderWithProxy, AmountFee, SignArea2, Warning, WrongPasswordAlert } from '../../../components';
import { useInfo, useProxies, useTranslation } from '../../../hooks';
import { useEstimatedFee, useInfo, useProxies, useTranslation } from '../../../hooks';
import { SubTitle } from '../../../partials';
import { PROXY_TYPE } from '../../../util/constants';
import { amountToHuman } from '../../../util/utils';
Expand Down Expand Up @@ -56,7 +55,6 @@ export default function Review ({ address, api, classToUnlock, setDisplayPopup,
const [selectedProxy, setSelectedProxy] = useState<Proxy | undefined>();
const [proxyItems, setProxyItems] = useState<ProxyItem[]>();
const [txInfo, setTxInfo] = useState<TxInfo | undefined>();
const [estimatedFee, setEstimatedFee] = useState<Balance>();
const [params, setParams] = useState<SubmittableExtrinsic<'promise', ISubmittableResult>[]>();
const [step, setStep] = useState<number>(STEPS.INDEX);

Expand All @@ -67,6 +65,8 @@ export default function Review ({ address, api, classToUnlock, setDisplayPopup,
const unlockClass = api.tx['convictionVoting']['unlock']; // (class)
const batchAll = api.tx['utility']['batchAll'];

const estimatedFee = useEstimatedFee(address, batchAll(params));
Nick-1979 marked this conversation as resolved.
Show resolved Hide resolved

const extraInfo = useMemo(() => ({
action: 'Unlock Referenda',
amount,
Expand All @@ -93,12 +93,6 @@ export default function Review ({ address, api, classToUnlock, setDisplayPopup,
const params = [...removes, ...unlocks];

setParams(params);

if (!api?.call?.['transactionPaymentApi']) {
return setEstimatedFee(api?.createType('Balance', BN_ONE) as Balance | undefined);
}

batchAll(params).paymentInfo(formatted).then((i) => setEstimatedFee(i?.partialFee)).catch(console.error);
}, [api, batchAll, formatted, classToUnlock, remove, unlockClass]);

useEffect((): void => {
Expand Down
4 changes: 2 additions & 2 deletions packages/extension-polkagate/src/fullscreen/governance/delegate/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ export function Delegate ({ address, open, setOpen, showDelegationNote }: Props)

batch(txList)
.paymentInfo(delegateInformation.delegateeAddress)
.then((i) => setEstimatedFee(i?.partialFee))
.then((i) => setEstimatedFee(api?.createType('Balance', i?.partialFee) as Balance))
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Based on the search results, I can now generate the final response:

Inconsistent Balance type handling detected in fee estimation

The codebase shows inconsistent handling of partial fees, where some components properly create Balance types while others directly use the raw partial fee:

  • ✓ Properly typed in governance/post/castVote/Cast.tsx
  • ✓ Properly typed in governance/delegate/modify/ModifyDelegate.tsx
  • ✓ Properly typed in governance/delegate/remove/RemoveDelegate.tsx
  • ✓ Properly typed in staking/solo/stake/index.tsx
  • ❌ Missing Balance type in sendFund/InputPage.tsx
  • ❌ Missing Balance type in staking/solo/tuneUp/index.tsx
  • ❌ Missing Balance type in crowdloans/contribute/Contribute.tsx

Consider applying the same type safety pattern api?.createType('Balance', partialFee) consistently across all fee estimations.

🔗 Analysis chain

LGTM! Type safety improvement for fee estimation.

The changes properly ensure type safety by creating a Balance type from the partial fee. This is a good practice as it maintains consistent typing throughout the application.

Let's verify the Balance type usage across the codebase:

Also applies to: 232-232

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for consistent Balance type usage in fee estimation
# Look for other instances where partial fees might need similar type safety

# Search for fee estimation patterns
rg -A 2 'paymentInfo.*then.*partialFee'

# Search for Balance type creation patterns
ast-grep --pattern 'createType("Balance", $_)'

Length of output: 6232

.catch(console.error);
} else {
const tx = delegate(...[
Expand All @@ -229,7 +229,7 @@ export function Delegate ({ address, open, setOpen, showDelegationNote }: Props)

tx
.paymentInfo(delegateInformation.delegateeAddress)
.then((i) => setEstimatedFee(i?.partialFee))
.then((i) => setEstimatedFee(api?.createType('Balance', i?.partialFee) as Balance))
.catch(console.error);
}
}, [api, batch, delegate, delegateInformation, delegateInformation?.delegateeAddress]);
Expand Down
39 changes: 25 additions & 14 deletions packages/extension-polkagate/src/fullscreen/governance/delegate/modify/ModifyDelegate.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,33 +1,35 @@
// Copyright 2019-2024 @polkadot/extension-polkagate authors & contributors
// SPDX-License-Identifier: Apache-2.0
// @ts-nocheck

//@ts-nocheck
Nick-1979 marked this conversation as resolved.
Show resolved Hide resolved
/* eslint-disable react/jsx-max-props-per-line */

/**
* @description
* this component opens Modify Delegate review page
* */

import type { SubmittableExtrinsic } from '@polkadot/api/types';
import type { Balance } from '@polkadot/types/interfaces';
import type { ISubmittableResult } from '@polkadot/types/types';
import type { BN } from '@polkadot/util';
import type { Lock } from '../../../../hooks/useAccountLocks';
import type { BalancesInfo, Proxy, TxInfo } from '../../../../util/types';
import type { AlreadyDelegateInformation, DelegateInformation } from '..';

import { Divider, Grid, Typography } from '@mui/material';
import React, { useCallback, useEffect, useMemo, useRef, useState } from 'react';

import { SubmittableExtrinsic } from '@polkadot/api/types';
import { ISubmittableResult } from '@polkadot/types/types';
import { BN, BN_ONE, BN_ZERO } from '@polkadot/util';
import { BN_ONE, BN_ZERO } from '@polkadot/util';

import { Identity, Motion, ShowValue, SignArea2, WrongPasswordAlert } from '../../../../components';
import { useCurrentBlockNumber, useIdentity, useInfo, useTracks, useTranslation } from '../../../../hooks';
import { Lock } from '../../../../hooks/useAccountLocks';
import { ThroughProxy } from '../../../../partials';
import { PROXY_TYPE } from '../../../../util/constants';
import { amountToHuman, amountToMachine } from '../../../../util/utils';
import DisplayValue from '../../post/castVote/partial/DisplayValue';
import TracksList from '../partial/TracksList';
import { AlreadyDelegateInformation, DelegateInformation, STEPS } from '..';
import { STEPS } from '..';
import Modify from './Modify';

interface Props {
Expand All @@ -51,7 +53,7 @@ interface Props {

export type ModifyModes = 'Modify' | 'ReviewModify';

export default function ModifyDelegate({ accountLocks, address, balances, classicDelegateInformation, formatted, lockedAmount, mixedDelegateInformation, mode, otherDelegatedTracks, selectedProxy, setDelegateInformation, setModalHeight, setMode, setStep, setTxInfo, step }: Props): React.ReactElement<Props> {
export default function ModifyDelegate ({ accountLocks, address, balances, classicDelegateInformation, formatted, lockedAmount, mixedDelegateInformation, mode, otherDelegatedTracks, selectedProxy, setDelegateInformation, setModalHeight, setMode, setStep, setTxInfo, step }: Props): React.ReactElement<Props> {
const { t } = useTranslation();
const { api, chain, decimal, genesisHash, token } = useInfo(address);
const { tracks } = useTracks(address);
Expand All @@ -76,11 +78,19 @@ export default function ModifyDelegate({ accountLocks, address, balances, classi
const [newConviction, setNewConviction] = useState<number | undefined>();

const selectedProxyAddress = selectedProxy?.delegate as unknown as string;
const undelegate = api && api.tx['convictionVoting']['undelegate'];
const delegate = api && api.tx['convictionVoting']['delegate'];
const batch = api && api.tx['utility']['batchAll'];

const acceptableConviction = useMemo(() => newConviction !== undefined ? newConviction === 0.1 ? 0 : newConviction : conviction === 0.1 ? 0 : conviction, [conviction, newConviction]);
const undelegate = api?.tx['convictionVoting']['undelegate'];
const delegate = api?.tx['convictionVoting']['delegate'];
const batch = api?.tx['utility']['batchAll'];

const acceptableConviction = useMemo(() =>
newConviction !== undefined
? newConviction === 0.1
? 0
: newConviction
: conviction === 0.1
? 0
: conviction
, [conviction, newConviction]);

const delegateAmountBN = useMemo(() => newDelegateAmount ? amountToMachine(newDelegateAmount, decimal) : classicDelegateInformation ? classicDelegateInformation.delegateAmountBN : BN_ZERO, [classicDelegateInformation, decimal, newDelegateAmount]);
const delegatePower = useMemo(() => {
Expand Down Expand Up @@ -109,6 +119,7 @@ export default function ModifyDelegate({ accountLocks, address, balances, classi

useEffect(() => {
if (ref) {
// @ts-ignore
setModalHeight(ref.current?.offsetHeight as number);
}
}, [setModalHeight]);
Expand Down Expand Up @@ -185,8 +196,8 @@ export default function ModifyDelegate({ accountLocks, address, balances, classi
}

txList.length === 1
? txList[0].paymentInfo(formatted).then((i) => setEstimatedFee(i?.partialFee)).catch(console.error)
: batch(txList).paymentInfo(formatted).then((i) => setEstimatedFee(i?.partialFee)).catch(console.error);
? txList[0].paymentInfo(formatted).then((i) => setEstimatedFee(api?.createType('Balance', i?.partialFee))).catch(console.error)
: batch(txList).paymentInfo(formatted).then((i) => setEstimatedFee(api?.createType('Balance', i?.partialFee))).catch(console.error);
Nick-1979 marked this conversation as resolved.
Show resolved Hide resolved
}, [api, batch, formatted, txList, undelegate]);

const extraInfo = useMemo(() => ({
Expand Down
4 changes: 2 additions & 2 deletions packages/extension-polkagate/src/fullscreen/governance/delegate/remove/RemoveDelegate.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,8 @@ export default function RemoveDelegate ({ address, classicDelegateInformation, f
}

params.length === 1
? undelegate(BN_ZERO).paymentInfo(formatted).then((i) => setEstimatedFee(i?.partialFee)).catch(console.error)
: batch(params.map((param) => undelegate(param))).paymentInfo(formatted).then((i) => setEstimatedFee(i?.partialFee)).catch(console.error);
? undelegate(BN_ZERO).paymentInfo(formatted).then((i) => setEstimatedFee(api?.createType('Balance', i?.partialFee) as Balance)).catch(console.error)
: batch(params.map((param) => undelegate(param))).paymentInfo(formatted).then((i) => setEstimatedFee(api?.createType('Balance', i?.partialFee) as Balance)).catch(console.error);
}, [api, batch, formatted, params, setSelectedTracksLength, undelegate]);

const extraInfo = useMemo(() => ({
Expand Down
2 changes: 1 addition & 1 deletion packages/extension-polkagate/src/fullscreen/governance/post/castVote/Cast.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ export default function Cast ({ address, notVoted, previousVote, refIndex, setSt
const dummyVote = undefined;
const feeDummyParams = ['1', dummyVote];

tx(...feeDummyParams).paymentInfo(formatted).then((i) => setEstimatedFee(i?.partialFee)).catch(console.error);
tx(...feeDummyParams).paymentInfo(formatted).then((i) => setEstimatedFee(api?.createType('Balance', i?.partialFee))).catch(console.error);
}, [api, formatted, tx]);

useEffect(() => {
Expand Down
24 changes: 4 additions & 20 deletions packages/extension-polkagate/src/fullscreen/governance/post/castVote/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

/* eslint-disable react/jsx-max-props-per-line */

import type { Balance } from '@polkadot/types/interfaces';
import type { Proxy, ProxyItem, TxInfo } from '../../../../util/types';
import type { Vote } from '../myVote/util';

Expand All @@ -12,10 +11,10 @@ import React, { useCallback, useEffect, useMemo, useState } from 'react';

import { AccountsStore } from '@polkadot/extension-base/stores';
import keyring from '@polkadot/ui-keyring';
import { BN, BN_ONE, noop } from '@polkadot/util';
import { BN, noop } from '@polkadot/util';
import { cryptoWaitReady } from '@polkadot/util-crypto';

import { useInfo, useProxies, useTranslation } from '../../../../hooks';
import { useEstimatedFee, useInfo, useProxies, useTranslation } from '../../../../hooks';
import { PROXY_TYPE } from '../../../../util/constants';
import { amountToHuman, amountToMachine } from '../../../../util/utils';
import { DraggableModalWithTitle } from '../../components/DraggableModalWithTitle';
Expand Down Expand Up @@ -76,7 +75,6 @@ export default function Index ({ address, cantModify, hasVoted, myVote, notVoted
const [selectedProxy, setSelectedProxy] = useState<Proxy | undefined>();
const [proxyItems, setProxyItems] = useState<ProxyItem[]>();
const [txInfo, setTxInfo] = useState<TxInfo | undefined>();
const [estimatedFee, setEstimatedFee] = useState<Balance>();
const [voteInformation, setVoteInformation] = useState<VoteInformation | undefined>();
const [step, setStep] = useState<number>(showAbout ? STEPS.ABOUT : STEPS.CHECK_SCREEN);
const [alterType, setAlterType] = useState<'modify' | 'remove'>();
Expand All @@ -85,6 +83,8 @@ export default function Index ({ address, cantModify, hasVoted, myVote, notVoted
const voteTx = api?.tx['convictionVoting']['vote'];
const removeTx = api?.tx['convictionVoting']['removeVote'];

const estimatedFee = useEstimatedFee(address, voteTx, ['1', undefined]);

Comment on lines +86 to +87
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Critical: Fee estimation parameters need revision

The current implementation has potential issues:

  1. The hardcoded parameters ['1', undefined] may not accurately reflect the actual transaction parameters
  2. Missing error handling for fee estimation failures

Consider updating the implementation to:

- const estimatedFee = useEstimatedFee(address, voteTx, ['1', undefined]);
+ const estimatedFee = useEstimatedFee(address, voteTx, [
+   voteInformation?.voteAmountBN?.toString(),
+   voteInformation?.voteConvictionValue
+ ]);

Also, add error handling:

if (!estimatedFee) {
  console.error('Failed to estimate transaction fee');
  // Handle the error appropriately
}

useEffect((): void => {
const fetchedProxyItems = proxies?.map((p: Proxy) => ({ proxy: p, status: 'current' })) as ProxyItem[];

Expand Down Expand Up @@ -149,22 +149,6 @@ export default function Index ({ address, cantModify, hasVoted, myVote, notVoted
}
}, [step, hasVoted, alterType, t]);

useEffect(() => {
if (!formatted || !voteTx) {
return;
}

if (!api?.call?.['transactionPaymentApi']) {
// eslint-disable-next-line @typescript-eslint/no-unsafe-return
return setEstimatedFee(api?.createType('Balance', BN_ONE) as Balance);
}

const dummyVote = undefined;
const feeDummyParams = ['1', dummyVote];

voteTx(...feeDummyParams).paymentInfo(formatted).then((i) => setEstimatedFee(i?.partialFee)).catch(console.error);
}, [api, formatted, voteTx]);

const handleClose = useCallback(() => {
if (step === STEPS.PROXY) {
setStep(alterType === 'remove' ? STEPS.REMOVE : STEPS.REVIEW);
Expand Down
31 changes: 7 additions & 24 deletions packages/extension-polkagate/src/fullscreen/governance/post/decisionDeposit/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,27 +1,23 @@
// Copyright 2019-2024 @polkadot/extension-polkagate authors & contributors
// SPDX-License-Identifier: Apache-2.0
// @ts-nocheck

/* eslint-disable react/jsx-max-props-per-line */

import type { Balance } from '@polkadot/types/interfaces';
import type { Proxy, ProxyItem, TxInfo } from '../../../../util/types';

import { Close as CloseIcon } from '@mui/icons-material';
import { Grid, Typography, useTheme } from '@mui/material';
import React, { useCallback, useEffect, useMemo, useState } from 'react';

import { AccountsStore } from '@polkadot/extension-base/stores';
import keyring from '@polkadot/ui-keyring';
import { BN_ONE } from '@polkadot/util';
import { cryptoWaitReady } from '@polkadot/util-crypto';

import { Identity, ShowBalance, SignArea2, Warning } from '../../../../components';
import { useAccountDisplay, useBalances, useInfo, useProxies, useTranslation } from '../../../../hooks';
import { useAccountDisplay, useBalances, useEstimatedFee, useInfo, useProxies, useTranslation } from '../../../../hooks';
import { ThroughProxy } from '../../../../partials';
import { getValue } from '../../../../popup/account/util';
import { Proxy, ProxyItem, TxInfo } from '../../../../util/types';
import { PROXY_TYPE } from '../../../../util/constants';
import type { Proxy, ProxyItem, TxInfo } from '../../../../util/types';
import { DraggableModal } from '../../components/DraggableModal';
import SelectProxyModal2 from '../../components/SelectProxyModal2';
import WaitScreen from '../../partials/WaitScreen';
Expand Down Expand Up @@ -61,27 +57,12 @@ export default function DecisionDeposit ({ address, open, refIndex, setOpen, tra
const [step, setStep] = useState<number>(STEPS.REVIEW);
const [txInfo, setTxInfo] = useState<TxInfo | undefined>();
const [isPasswordError, setIsPasswordError] = useState(false);
const [estimatedFee, setEstimatedFee] = useState<Balance>();
const [selectedProxy, setSelectedProxy] = useState<Proxy | undefined>();

const tx = api && api.tx['referenda']['placeDecisionDeposit'];
const tx = api?.tx['referenda']['placeDecisionDeposit'];
const amount = track?.[1]?.decisionDeposit;
const selectedProxyAddress = selectedProxy?.delegate as unknown as string;

useEffect(() => {
if (!formatted || !tx) {
return;
}

if (!api?.call?.['transactionPaymentApi']) {
// eslint-disable-next-line @typescript-eslint/no-unsafe-return
return setEstimatedFee(api?.createType('Balance', BN_ONE));
}

const feeDummyParams = [1];

tx(...feeDummyParams).paymentInfo(formatted).then((i) => setEstimatedFee(i?.partialFee)).catch(console.error);
}, [api, formatted, tx]);
const estimatedFee = useEstimatedFee(address, tx, [1]);
Nick-1979 marked this conversation as resolved.
Show resolved Hide resolved

useEffect(() => {
cryptoWaitReady().then(() => keyring.loadAll({ store: new AccountsStore() })).catch(() => null);
Expand Down Expand Up @@ -121,6 +102,8 @@ export default function DecisionDeposit ({ address, open, refIndex, setOpen, tra
if (step === STEPS.CONFIRM) {
return t('Paying Confirmation');
}

return undefined;
}, [step, t]);

const HEIGHT = 550;
Expand Down Expand Up @@ -193,7 +176,7 @@ export default function DecisionDeposit ({ address, open, refIndex, setOpen, tra
</Grid>
<Grid container item sx={{ bottom: '20px', left: '4%', position: 'absolute', width: '92%' }}>
<SignArea2
address={address as string}
address={address!}
Nick-1979 marked this conversation as resolved.
Show resolved Hide resolved
call={tx}
disabled={notEnoughBalance}
extraInfo={extraInfo}
Expand Down
Loading
Loading