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

Jump to bottom

chore(suite): use structuredClone for deep copying #6071

Closed
wants to merge 4 commits into from
Closed

chore(suite): use structuredClone for deep copying #6071

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
208ba56
feat(suite): use structuredClone for deep copying
dahaca Aug 22, 2022
6e3d10d
fixup! feat(suite): use structuredClone for deep copying
dahaca Aug 23, 2022
c21c178
fixup! feat(suite): use structuredClone for deep copying
dahaca Aug 24, 2022
da91863
fixup! feat(suite): use structuredClone for deep copying
dahaca Aug 24, 2022
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
22 changes: 6 additions & 16 deletions packages/connect/src/data/coinInfo.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// origin: https:/trezor/connect/blob/develop/src/js/data/CoinInfo.js

import { deepClone } from '@trezor/utils';
import { ERRORS } from '../constants';
import { toHardened, fromHardened } from '../utils/pathUtils';
import type {
Expand All @@ -13,19 +14,8 @@ const bitcoinNetworks: BitcoinNetworkInfo[] = [];
const ethereumNetworks: EthereumNetworkInfo[] = [];
const miscNetworks: MiscNetworkInfo[] = [];

// TODO: replace by structuredClone() after updating TS
export function cloneCoinInfo<T>(info: T): T {
const jsonString = JSON.stringify(info);
if (jsonString === undefined) {
// jsonString === undefined IF and only IF obj === undefined
// therefore no need to clone
return info;
}
return JSON.parse(jsonString);
}

export const getBitcoinNetwork = (pathOrName: number[] | string) => {
const networks = cloneCoinInfo(bitcoinNetworks);
const networks: BitcoinNetworkInfo[] = deepClone(bitcoinNetworks);
if (typeof pathOrName === 'string') {
const name = pathOrName.toLowerCase();
return networks.find(
Expand All @@ -40,7 +30,7 @@ export const getBitcoinNetwork = (pathOrName: number[] | string) => {
};

export const getEthereumNetwork = (pathOrName: number[] | string) => {
const networks = cloneCoinInfo(ethereumNetworks);
const networks: EthereumNetworkInfo[] = deepClone(ethereumNetworks);
if (typeof pathOrName === 'string') {
const name = pathOrName.toLowerCase();
return networks.find(
Expand All @@ -52,7 +42,7 @@ export const getEthereumNetwork = (pathOrName: number[] | string) => {
};

export const getMiscNetwork = (pathOrName: number[] | string) => {
const networks = cloneCoinInfo(miscNetworks);
const networks: MiscNetworkInfo[] = deepClone(miscNetworks);
if (typeof pathOrName === 'string') {
const name = pathOrName.toLowerCase();
return networks.find(
Expand Down Expand Up @@ -95,7 +85,7 @@ export const getBech32Network = (coin: BitcoinNetworkInfo) => {

// fix coinInfo network values from path (segwit/legacy)
export const fixCoinInfoNetwork = (ci: BitcoinNetworkInfo, path: number[]) => {
const coinInfo = cloneCoinInfo(ci);
const coinInfo: BitcoinNetworkInfo = deepClone(ci);
if (path[0] === toHardened(84)) {
const bech32Network = getBech32Network(coinInfo);
if (bech32Network) {
Expand Down Expand Up @@ -131,7 +121,7 @@ const detectBtcVersion = (data: { subversion?: string }) => {

// TODO: https:/trezor/trezor-suite/issues/4886
export const getCoinInfoByHash = (hash: string, networkInfo: any) => {
const networks = cloneCoinInfo(bitcoinNetworks);
const networks: BitcoinNetworkInfo[] = deepClone(bitcoinNetworks);
const result = networks.find(
info => hash.toLowerCase() === info.hashGenesisBlock.toLowerCase(),
);
Expand Down
17 changes: 3 additions & 14 deletions packages/suite/src/actions/wallet/sendFormActions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import TrezorConnect, { PROTO } from '@trezor/connect';
import BigNumber from 'bignumber.js';
import { deepClone } from '@trezor/utils';

import * as accountActions from '@wallet-actions/accountActions';
import * as blockchainActions from '@wallet-actions/blockchainActions';
import * as transactionActions from '@wallet-actions/transactionActions';
Expand Down Expand Up @@ -99,19 +101,6 @@ export const removeDraft = () => (dispatch: Dispatch, getState: GetState) => {
}
};

// TODO: replace by structuredClone() after updating TS
const clone = <T>(info: T): T => {
const jsonString = JSON.stringify(info);

if (jsonString === undefined) {
// jsonString === undefined IF and only IF obj === undefined
// therefore no need to clone
return info;
}

return JSON.parse(jsonString);
};

export const convertDrafts = () => (dispatch: Dispatch, getState: GetState) => {
const { route } = getState().router;

Expand Down Expand Up @@ -146,7 +135,7 @@ export const convertDrafts = () => (dispatch: Dispatch, getState: GetState) => {
const conversionToUse =
areSatsSelected && areSatsSupported ? amountToSatoshi : formatAmount;

const updatedDraft = clone(draft);
const updatedDraft: FormState = deepClone(draft);
const decimals = getAccountDecimals(relatedAccount.symbol)!;

updatedDraft.outputs.forEach(output => {
Expand Down
15 changes: 15 additions & 0 deletions packages/utils/src/deepClone.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
export const deepClone = <T>(value: T): T => {
Copy link
Member

Choose a reason for hiding this comment

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

There are ~ 12 more places where JSON.parse(JSON.stringify... is used, I guess it would be nice to use this util on all those places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

da91863

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest little bit restrict values that can be passed, because you can pass anything now I think, but I am not sure how JSON will behave if you will pass object with BigInt or other special types. It will work for structuredClone but JSON could mess it up, so we should limit it only types that will work 100% for both solutions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And now we are back to square 1 from which the conversation has started hehe :D Yeah, makes sense.

Copy link
Member

@karliatto karliatto Aug 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I agree that we should consider that objects are going to be stringified when using JSON.pasrse(JSON.stringify....
I am not worried about current places where it is used (because it is already working) but in potential new uses where we think that this behaves like structuredClone and in in some scenarios it will not.

const appointment = {
    location: 'Prague',
    day: new Date(),
}

const whenStructuredClone = structuredClone(appointment)
whenStructuredClone.day.getDate()
// result -> 25

const whenStringified = JSON.parse(JSON.stringify(appointment))
whenStringified.day.getDate()
// result -> Uncaught TypeError: whenStringified.day.getDate is not a function

And the same for nested objects. And the same behavior can be expected from other objects like BigInt.

Restricting the use to types that will work 100% for both could be a solution but if I am not mistaken I think it should iterate over the whole object and nested potential objects to make sure there is no nested BigInt or other not working type.

Copy link
Contributor

@Nodonisko Nodonisko Aug 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think type like this should do the job:

import { JsonValue } from 'type-fest'

if (value === undefined) {
return value;
}

let clone;

if (typeof structuredClone === 'function') {
clone = structuredClone(value);
} else {
clone = JSON.parse(JSON.stringify(value));
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably use some better polyfill than this. There are already available.

Copy link
Member

Choose a reason for hiding this comment

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

Never heard of it before, core-js has a polyfill. (I know we are cautious about included new dependencies, but just sharing).

https:/zloirock/core-js#structuredclone

}

return clone;
};
1 change: 1 addition & 0 deletions packages/utils/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,4 @@ export * from './throwError';
export * from './isHex';
export * from './resolveStaticPath';
export * from './createTimeoutPromise';
export * from './deepClone';
24 changes: 24 additions & 0 deletions packages/utils/tests/deepClone.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import { deepClone } from '../src/deepClone';

describe('deepClone', () => {
it('deepClone works', () => {
const original = {
a: 1,
b: '2',
c: {
a: 1,
b: '2',
c: {
a: [1, 2, 3],
b: '2',
},
},
};

const copy = deepClone(original);
const shallowCopy = { ...original };

expect(copy.c.c.a === original.c.c.a).toBeFalsy();
expect(shallowCopy.c.c.a === original.c.c.a).toBeTruthy();
});
});