refactor(abstract-eth): move methods to abstract-eth #4039

gianchandania · 2023-11-07T18:33:29Z

This PR moves the common EthLike methods to AbstractEthLikeNewCoins class. EthLikeToken also inherits from this new class. ETH and Polygon classes have been modified to inherit from this new class. All the tokens like BscToken, PolygonToken are inheriting EthLikeToken class, but Erc20Token is still inheriting Eth class because ERC20 tokens use some of the methods defined in the Eth class like signing methods

WIN-1012
TICKET: WIN-1012

gianchandania · 2023-11-08T04:53:41Z

modules/abstract-eth/src/abstractEthLikeNewCoins.ts

+  signingKeyNonce?: number;
+  walletContractAddress?: string;
+  prv: string;
+  recipients?: Recipient[];


Here I had to make all the fields optional because I was getting an error that the params passed to signTransaction are not compatible as we also have signTransaction in AbstractEthLikeCoin class and the interface used there is not having all these fields.

Moving from required to optional isnt a breaking change right?

Correct, will remove this from breaking change

gianchandania · 2023-11-08T04:56:08Z

modules/abstract-eth/src/abstractEthLikeNewCoins.ts

+  gasLimit?: number;
+  gasPrice?: number;
+  custodianTransactionId?: string;
+}


Had to make all fields optional because of the same issue mentioned above with signTransaction method

gianchandania · 2023-11-08T09:01:23Z

modules/abstract-eth/src/abstractEthLikeCoin.ts

  txInfo: TxInfo;
  feeInfo: EthTransactionFee;
  source: string;
  dataToSign: string;
-  nextContractSequenceId?: string;
+  nextContractSequenceId?: number;


BREAKING CHANGE: Type of nextContractSequenceId is changed to number to avoid typescript error of incompatible function params for signTransaction method in AbstractEthLikeNewCoins and AbstractEthLikeCoin

Have created a ticket WIN-1098 to address the change required in OVC because of this

gianchandania · 2023-11-08T09:03:34Z

modules/abstract-eth/src/abstractEthLikeNewCoins.ts

 export interface TransactionPrebuild extends BaseTransactionPrebuild {
  hopTransaction?: HopPrebuild;
  buildParams: {
    recipients: Recipient[];
  };
  recipients: TransactionRecipient[];
-  nextContractSequenceId: string;
+  nextContractSequenceId: number;


BREAKING CHANGE: Type of nextContractSequenceId is changed to number to avoid typescript error of incompatible function params for signTransaction method in AbstractEthLikeNewCoins and AbstractEthLikeCoin

gianchandania · 2023-11-08T15:34:44Z

modules/sdk-coin-polygon/src/polygon.ts

-   * @param params.prv
-   * @returns {{txHex: string}}
-   */
-  async signFinalPolygon(params: SignFinalOptions): Promise<FullySignedTransaction> {


BREAKING CHANGE: signFinalPolygon method name is updated to signFinalEthLike so that it can be used for other EthLike coins

gianchandania · 2023-11-08T15:37:04Z

modules/sdk-coin-polygon/src/polygon.ts

-      return 'PolygonMumbai';
-    }
-    return 'PolygonMainnet';
-  }


BREAKING CHANGE: getCustomChainName method is removed and getCustomChainCommon method is added in the AbstractEthLikeNewCoins class to return ethlike common object

gianchandania · 2023-11-08T15:41:01Z

modules/sdk-coin-polygon/src/polygon.ts

-    const defaultHardfork = !!params.eip1559 ? 'london' : optionalDeps.EthCommon.Hardfork.TangerineWhistle;
-    let customChainCommon;
-    // if replay protection options are set, override the default common setting
-    if (params.replayProtectionOptions) {


BREAKING CHANGE: replayProtectionOptions is not optional in buildTransaction method for EthLike coins and needs to be passed to derive the Eth common object from the chainId

zahin-mohammad

Will reserve a final review until @BitGo/ethalt-team reviews.

Also for breaking change commits please ensure that its the last grouping with no spaced lines.

Example:

refactor(abstract-eth): move methods to abstract-eth

<description>

BREAKING CHANGE: something changed
TICKET: WIN-1012

dpkjnr · 2023-11-09T05:25:56Z

modules/abstract-eth/src/lib/resources.ts

@@ -0,0 +1,16 @@
+// Mapping of all supported coins with their chainIds
+export const EthLikeCoinNameFromChainId = {


can we create this map using statics?

sachushaji

Nice Work!!!. Some small NITs

sachushaji · 2023-11-09T06:14:32Z

modules/abstract-eth/src/abstractEthLikeNewCoins.ts

+        throw new Error('Invalid address: ' + recipient.address);
+      }
+
+      let amount;


Suggested change

let amount;

let amount: BigNumber;

sachushaji · 2023-11-09T06:18:26Z

modules/abstract-eth/src/abstractEthLikeNewCoins.ts

   * @param address
+   * @returns {*}


Suggested change

* @returns {*}

* @returns {Promise<number>}

sachushaji · 2023-11-09T06:19:23Z

modules/abstract-eth/src/abstractEthLikeNewCoins.ts

+   * @param txInfo
+   * @param ethTx
+   * @param userKey
+   * @param backupKey
+   * @param gasPrice
+   * @param gasLimit
+   * @param eip1559
+   * @param replayProtectionOptions
+   * @returns {Promise<OfflineVaultTxInfo>}


Lets add JSDOC consistently with all other functions in the file

modules/sdk-coin-eth/src/eth.ts

gianchandania · 2023-11-10T07:42:01Z

Will reserve a final review until @BitGo/ethalt-team reviews.

Also for breaking change commits please ensure that its the last grouping with no spaced lines.

Example:
refactor(abstract-eth): move methods to abstract-eth

<description>

BREAKING CHANGE: something changed
TICKET: WIN-1012

removed blank spaces from BREAKING CHANGE description

sachushaji

Awesome!! 👍

gianchandania · 2023-11-10T10:20:40Z

Squashed the commits

WIN-1012 BREAKING CHANGE: Type of nextContractSequenceId field in TransactionPrebuild interface is changed from string to number in AbstractEthLikeCoin and AbstractEthLikeNewCoins classes. getCustomChainName method is removed from Polygon class because a common method getCustomChainCommon has been added to AbstractEthLikeNewCoins class for all EthLike coins. replayProtectionOptions is not optional in buildTransaction method in AbstractEthLikeNewCoins and needs to be passed to derive the Eth common object from the chainId. signFinalPolygon method name from Polygon class is updated to signFinalEthLike so that it can be used for other EthLike coins. getBaseFactor method in Eth and Polygon class returns number instead of string just to align with AbstractEthLikeCoin Ticket: WIN-1012

gianchandania commented Nov 8, 2023

View reviewed changes

gianchandania force-pushed the WIN-1012-move-methods-from-sdk-coin-eth branch 3 times, most recently from 09bb52f to 00ac185 Compare November 8, 2023 14:43

gianchandania commented Nov 8, 2023

View reviewed changes

gianchandania force-pushed the WIN-1012-move-methods-from-sdk-coin-eth branch 2 times, most recently from edb0910 to 9a2ec21 Compare November 8, 2023 15:58

gianchandania marked this pull request as ready for review November 8, 2023 16:42

gianchandania requested review from a team as code owners November 8, 2023 16:42

zahin-mohammad reviewed Nov 9, 2023

View reviewed changes

dpkjnr reviewed Nov 9, 2023

View reviewed changes

sachushaji previously approved these changes Nov 9, 2023

View reviewed changes

gianchandania force-pushed the WIN-1012-move-methods-from-sdk-coin-eth branch 3 times, most recently from 285d428 to 3b72cb5 Compare November 9, 2023 11:36

gianchandania dismissed sachushaji’s stale review via 9020895 November 10, 2023 08:30

gianchandania force-pushed the WIN-1012-move-methods-from-sdk-coin-eth branch from b7eaec3 to 42c2a6c Compare November 10, 2023 08:34

gianchandania requested review from sachushaji, zahin-mohammad and dpkjnr November 10, 2023 09:28

sachushaji previously approved these changes Nov 10, 2023

View reviewed changes

gianchandania force-pushed the WIN-1012-move-methods-from-sdk-coin-eth branch from 42c2a6c to 1921f62 Compare November 10, 2023 10:20

dpkjnr previously approved these changes Nov 10, 2023

View reviewed changes

gianchandania dismissed stale reviews from dpkjnr and sachushaji via a25ee6a November 10, 2023 17:52

sachushaji approved these changes Nov 10, 2023

View reviewed changes

gianchandania force-pushed the WIN-1012-move-methods-from-sdk-coin-eth branch from a25ee6a to af8bd10 Compare November 10, 2023 17:59

angshal approved these changes Nov 10, 2023

View reviewed changes

gianchandania merged commit 5bc8c97 into master Nov 11, 2023
9 checks passed

This was referenced Jan 3, 2024

[Snyk] Security upgrade bitgo from 19.5.0 to 21.0.0 ajax000-ai/BitGoJS#26

Open

[Snyk] Security upgrade bitgo from 19.20.0 to 21.0.0 ajax000-ai/BitGoJS#27

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(abstract-eth): move methods to abstract-eth #4039

refactor(abstract-eth): move methods to abstract-eth #4039

gianchandania commented Nov 7, 2023 •

edited

Loading

gianchandania Nov 8, 2023 •

edited

Loading

zahin-mohammad Nov 9, 2023

gianchandania Nov 9, 2023

gianchandania Nov 8, 2023 •

edited

Loading

gianchandania Nov 8, 2023 •

edited

Loading

gianchandania Nov 9, 2023

gianchandania Nov 8, 2023

gianchandania Nov 8, 2023

gianchandania Nov 8, 2023

gianchandania Nov 8, 2023

zahin-mohammad left a comment

dpkjnr Nov 9, 2023

gianchandania Nov 10, 2023

sachushaji left a comment

sachushaji Nov 9, 2023

gianchandania Nov 9, 2023

sachushaji Nov 9, 2023

gianchandania Nov 9, 2023

sachushaji Nov 9, 2023

gianchandania Nov 10, 2023

gianchandania commented Nov 10, 2023

sachushaji left a comment

gianchandania commented Nov 10, 2023

		@@ -0,0 +1,16 @@
		// Mapping of all supported coins with their chainIds
		export const EthLikeCoinNameFromChainId = {

refactor(abstract-eth): move methods to abstract-eth #4039

refactor(abstract-eth): move methods to abstract-eth #4039

Conversation

gianchandania commented Nov 7, 2023 • edited Loading

gianchandania Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gianchandania Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

gianchandania Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zahin-mohammad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sachushaji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gianchandania commented Nov 10, 2023

sachushaji left a comment

Choose a reason for hiding this comment

gianchandania commented Nov 10, 2023

gianchandania commented Nov 7, 2023 •

edited

Loading

gianchandania Nov 8, 2023 •

edited

Loading

gianchandania Nov 8, 2023 •

edited

Loading

gianchandania Nov 8, 2023 •

edited

Loading