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

added support for big numbers and sending > 9007 XCH #642

Closed
wants to merge 20 commits into from
Closed

Conversation

seeden
Copy link
Contributor

@seeden seeden commented Mar 4, 2022

added support for big numbers and sending more than 9007 XCH
fixed formatting for big numbers

@lgtm-com
Copy link

lgtm-com bot commented Mar 4, 2022

This pull request introduces 7 alerts when merging 2fbdb3e into 672cf2a - view on LGTM.com

new alerts:

  • 7 for Unused variable, import, function or class

@paninaro
Copy link
Contributor

paninaro commented Mar 4, 2022

There are a small number of files that still use JSON.parse() and JSON.stringify(). Should we be using JSONbig everywhere, and if so, is there some way we can enforce its usage (custom ESLint warning?)

@@ -145,7 +146,8 @@ function generateTransactionGraphData(
TransactionType.OUTGOING_TRADE,
].includes(type);

const value = (amount + feeAmount) * (isOutgoing ? -1 : 1);
const total = BigNumber(amount).plus(BigNumber(feeAmount));
const value = isOutgoing ? total.negated() : total;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think some comment should be added.
I expect if isOutgoing === false then it is incoming, but readers may not be sure about this assumption.

@@ -196,8 +202,8 @@ function prepareGraphPoints(
const points = [
{
x: peakTransaction.confirmedAtHeight,
y: Math.max(0, Number(mojoToChia(start))),
tooltip: mojoToChia(balance),
y: BigNumber.max(0, mojoToChia(start)).toNumber(), // max 21,000,000 safe to number
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm doubtful about this // max 21,000,000 safe to number
image

But if this is used as only visualization, it causes no harm. Fixing comment will suffice.

@@ -208,8 +214,8 @@ function prepareGraphPoints(

points.push({
x: timestamp,
y: Math.max(0, Number(mojoToChia(start))),
tooltip: mojoToChia(start),
y: BigNumber.max(0, mojoToChia(start)).toNumber(), // max 21,000,000 safe to number
Copy link
Contributor

Choose a reason for hiding this comment

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

The same with above comment.

@@ -4,7 +4,7 @@ import { Trans } from '@lingui/macro';
import moment from 'moment';
import { Box, IconButton, Table as TableBase, TableBody, TableCell, TableRow, Tooltip, Typography, Chip } from '@material-ui/core';
import { CallReceived as CallReceivedIcon, CallMade as CallMadeIcon, ExpandLess as ExpandLessIcon, ExpandMore as ExpandMoreIcon } from '@material-ui/icons';
import { Card, CardKeyValue, CopyToClipboard, Flex, Loading, StateColor, TableControlled, toBech32m, useCurrencyCode, mojoToChiaLocaleString, mojoToCATLocaleString } from '@chia/core';
import { Card, CardKeyValue, CopyToClipboard, Flex, Loading, StateColor, TableControlled, toBech32m, useCurrencyCode, mojoToChia, mojoToCAT, FormatLargeNumber } from '@chia/core';
import { useGetOfferRecordMutation, useGetSyncStatusQuery } from '@chia/api-react';
import styled from 'styled-components';
import type { Row } from '@chia/core';
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems Row is not exported from @chia/core.

  • packages/core/index.ts
    export * from './components';
    export * from './constants';
    export * from './utils';
    export * from './hooks';
    export * from './theme';
    export * from './screens';
    export * as locales from './locales';  
    
  • packages/core/components/index.ts
    ...
    export { default as Table, TableControlled } from './Table';
    ...
    
  • packages/core/components/Table/index.ts
    export { default } from './Table';
    export { default as TableControlled } from './TableControlled';
    

@ChiaMineJP
Copy link
Contributor

ChiaMineJP commented Mar 6, 2022

packages/wallets/src/components/offers/OfferEditorRowData.ts

  • spendBalance should be BigNumber because in packages/wallets/src/components/offers/OfferEditorConditionsPanel.tsx, there is line:
    balance = mojoToChia(walletBalance.spendableBalance);
    ...
    row.spendableBalance = balance;
    

packages/wallets/src/components/offers/OfferEditorConditionsPanel.tsx

  • Line61: if (balanceString !== row.spendableBalanceString || balance !== row.spendableBalance) {
    balance is BigNumber so native !== does not work.

packages/gui/src/components/block/Block.jsx

  • Line307 ${baseFarmerReward} ${currencyCode}:
    baseFarmerReward is BigNumber, so by including in string template, BigNumber.toString() is implicitly called.
    BigNumber.toString() can be 2.5e-11. Is it allowed? If not please consider using BigNumber.toFixed()

packages/gui/src/components/farm/card/FarmCardBlockRewards.tsx

  • Need to check and update whole lines.

packages/wallets/src/components/did/WalletDID.tsx

  • Line 615. {mojoToChiaLocaleString(props.balance)} TXCH Is this OK? Maybe not relevant to this PR though.

packages/gui/src/util/chia.js

  • Replace big.js with bignumber.js for whole lines.

packages/gui/src/types/Transaction.ts

  • Shouldn't type of amount, sent and fee_amount be number|BigNumber?

packages/api-react/src/services/wallet.ts

  • Line263-264 BigInt should be replaced by BigNumber.
    image
    The value of PendingBalance will be later passed to useWalletHumanValue but it does not expect that value may be BigInt.
    image
  • There may be lines where BigNumber must be used foruint64/uint128 properties.

@lgtm-com
Copy link

lgtm-com bot commented Mar 6, 2022

This pull request introduces 7 alerts when merging 90ea447 into 672cf2a - view on LGTM.com

new alerts:

  • 7 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Mar 6, 2022

This pull request introduces 7 alerts when merging d00b450 into 672cf2a - view on LGTM.com

new alerts:

  • 7 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Mar 9, 2022

This pull request introduces 1 alert when merging e049ded into 85e2e98 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Mar 9, 2022

This pull request introduces 1 alert when merging 596c6d3 into f8f218c - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Mar 10, 2022

This pull request introduces 1 alert when merging 50dd2e6 into cdfa2b9 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

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.

3 participants