-
Notifications
You must be signed in to change notification settings - Fork 5.5k
fix: cp-13.10.1 cp-13.11.0 dapp swap fix conversion rate for pol native token #38102
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
Changes from all commits
9f500db
b1e371b
64c8956
95a3a69
613bfcf
e20d379
179964d
e7a4726
ccd13a6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,22 +1,22 @@ | ||
| import { BigNumber } from 'bignumber.js'; | ||
| import { getNativeTokenAddress } from '@metamask/assets-controllers'; | ||
| import { Hex } from '@metamask/utils'; | ||
| import { | ||
| getNativeAssetForChainId, | ||
| isNativeAddress, | ||
| } from '@metamask/bridge-controller'; | ||
| import { TransactionMeta } from '@metamask/transaction-controller'; | ||
| import { getNativeAssetForChainId } from '@metamask/bridge-controller'; | ||
| import { CHAIN_IDS, TransactionMeta } from '@metamask/transaction-controller'; | ||
| import { useCallback } from 'react'; | ||
|
|
||
| import { TokenStandAndDetails } from '../../../../../store/actions'; | ||
| import { fetchTokenExchangeRates } from '../../../../../helpers/utils/util'; | ||
| import { useAsyncResult } from '../../../../../hooks/useAsync'; | ||
| import { isNativeAddress } from '../../../../../helpers/utils/token-insights'; | ||
| import { | ||
| fetchAllTokenDetails, | ||
| getTokenValueFromRecord, | ||
| } from '../../../utils/token'; | ||
| import { useConfirmContext } from '../../../context/confirm'; | ||
|
|
||
| const POLYGON_NATIVE_ASSET = '0x0000000000000000000000000000000000001010'; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can delegate to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the function is returning different address
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should use |
||
|
|
||
| export function useDappSwapUSDValues({ | ||
| tokenAddresses = [], | ||
| destTokenAddress, | ||
|
|
@@ -32,11 +32,22 @@ export function useDappSwapUSDValues({ | |
|
|
||
| const { value: fiatRates, pending: fiatRatesPending } = useAsyncResult< | ||
| Record<Hex, number | undefined> | ||
| >(() => { | ||
| >(async () => { | ||
| const addresses = tokenAddresses.filter( | ||
| (tokenAddress) => !isNativeAddress(tokenAddress), | ||
| ); | ||
| return fetchTokenExchangeRates('usd', addresses as Hex[], chainId); | ||
| const exchangeRates = await fetchTokenExchangeRates( | ||
| 'usd', | ||
| addresses as Hex[], | ||
| chainId, | ||
| ); | ||
|
|
||
| if (chainId === CHAIN_IDS.POLYGON) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be encapsulated inside
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure of other code will need this too.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally the dedupe should be done in the controller side.. but I have just noticed that there was a massive breaking change added to the codefi-v2.ts in core https://github.com/MetaMask/core/pull/7119/files#diff-5801aea8052edaaf64cbe9cbe48734d6e35865a36f37d8a96c4e538536c7b1c2
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unit test?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep adding those
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. PR is upadated |
||
| const nativeAddress = getNativeAssetForChainId(chainId).address; | ||
| exchangeRates[nativeAddress] = exchangeRates[POLYGON_NATIVE_ASSET]; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the difference between
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Price service is using first one.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok the reason for it is because our Bridge API and the dapp swap considers the zero address as the native address for Polygon |
||
| } | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This fixes the issue for pol token only, I could not find a more generic fix.
jpuri marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| return exchangeRates; | ||
| }, [chainId, tokenAddresses?.length]); | ||
|
|
||
| const { value: tokenDetails, pending: tokenDetailsPending } = useAsyncResult< | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Test mocks unrequested data masking logic bug
The test mocks
fetchTokenExchangeRatesto return a rate for the Polygon native asset (0x...1010), even though the hook implementation does not request this address. This mismatch between the mock and the actual implementation masks the bug where the rate is never fetched in production.