feat(abstract-eth): add recover consolidation for eth #6550

abhi-bitgo · 2025-07-25T13:26:26Z

Adding the recover consolidation functionality for eth and hteth which would be used for unsigned consolidation in WRW

ticket: WIN-5700

mullapudipruthvik · 2025-08-01T06:50:03Z

@claude review this PR as Grug

Copilot

Pull Request Overview

This PR adds consolidation recovery functionality for Ethereum-based coins (ETH and HTETH) to support unsigned consolidation operations in WRW. The implementation includes methods to generate forwarder addresses, derive addresses from public keys, and recover funds from multiple consolidation addresses.

Key changes:

Added network configuration properties for wallet V4 forwarder addresses
Implemented consolidation recovery methods in the abstract Ethereum class
Added comprehensive address generation and scanning functionality

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
modules/statics/src/networks.ts	Added wallet V4 forwarder factory and implementation addresses to Ethereum and Holesky network configurations
modules/statics/test/unit/resources/amsTokenConfig.ts	Updated test configuration with new forwarder addresses
modules/abstract-eth/src/abstractEthLikeNewCoins.ts	Implemented core consolidation recovery functionality including address generation, derivation, and transaction building

Comments suppressed due to low confidence (1)

modules/abstract-eth/src/abstractEthLikeNewCoins.ts:1318

Using 'any' type reduces type safety. Consider defining a proper interface or union type for consolidation transactions.

    const consolidationTransactions: any[] = [];

Copilot · 2025-08-04T06:47:24Z

modules/abstract-eth/src/abstractEthLikeNewCoins.ts

+        );
+        possibleConsolidationAddresses.push(forwarderAddress);
+      } catch (e) {
+        console.log(`Failed to generate forwarder address: ${e.message}`);


Using console.log for error logging is not appropriate for production code. Consider using a proper logging framework or throwing/handling the error appropriately.

Suggested change

console.log(`Failed to generate forwarder address: ${e.message}`);

debug(`Failed to generate forwarder address: ${e instanceof Error ? e.message : e}`);

Copilot · 2025-08-04T06:47:24Z

modules/abstract-eth/src/abstractEthLikeNewCoins.ts

+        const derivedAddress = this.deriveAddressFromPublicKey(params.userKey, index);
+        possibleConsolidationAddresses.push(derivedAddress);
+      } catch (e) {
+        console.log(`Failed to generate derived address: ${e}`);


Using console.log for error logging is not appropriate for production code. Consider using a proper logging framework or throwing/handling the error appropriately.

Suggested change

console.log(`Failed to generate derived address: ${e}`);

debug(`Failed to generate derived address: ${e}`);

Copilot · 2025-08-04T06:47:24Z

modules/abstract-eth/src/abstractEthLikeNewCoins.ts

+        }
+      }
+
+      // lastScanIndex = i;


This commented out code should be removed or properly implemented if needed.

Suggested change

// lastScanIndex = i;

Copilot · 2025-08-04T06:47:25Z

modules/abstract-eth/src/abstractEthLikeNewCoins.ts

+            chain: params.replayProtectionOptions?.chain || 0,
+            hardfork: params.replayProtectionOptions?.hardfork || 'london',
+          },
+          bitgoKey: '',


Setting bitgoKey to an empty string may be problematic. Consider using a proper default value or making this parameter optional if it's not required.

Suggested change

bitgoKey: '',

...(params.bitgoKey !== undefined ? { bitgoKey: params.bitgoKey } : {}),

mullapudipruthvik · 2025-08-04T06:55:27Z

modules/abstract-eth/src/abstractEthLikeNewCoins.ts

+
+    if (params.userKey) {
+      try {
+        const derivedAddress = this.deriveAddressFromPublicKey(params.userKey, index);


derivedAddress is returned for every getConsolidationAddress call, why is that ?

We don't have any means to differentiate between v6 and v5 wallet thus we try to derive address every time.

in that case this would execute for v6 wallet as well right ?
const forwarderAddress = this.generateForwarderAddress( params.walletContractAddress, params.bitgoFeeAddress, forwarderFactoryAddress, forwarderImplementationAddress, index ); possibleConsolidationAddresses.push(forwarderAddress);

mullapudipruthvik · 2025-08-04T06:57:23Z

modules/abstract-eth/src/abstractEthLikeNewCoins.ts

+    const salt = addHexPrefix(index.toString(16));
+    const saltBuffer = setLengthLeft(toBuffer(salt), 32);
+
+    const { createForwarderParams, createForwarderTypes } = getCreateForwarderParamsAndTypes(


this is specific for v4 wallet, can you not make it generic ?

Since we are not sure which version is being used, we have kept this in a try statement in order to handle the failure

mullapudipruthvik

also please add test cases

mullapudipruthvik · 2025-08-08T04:55:05Z

modules/abstract-eth/src/abstractEthLikeNewCoins.ts

+
+    if (params.userKey) {
+      try {
+        const derivedAddress = this.deriveAddressFromPublicKey(params.userKey, index);


in that case this would execute for v6 wallet as well right ?
const forwarderAddress = this.generateForwarderAddress( params.walletContractAddress, params.bitgoFeeAddress, forwarderFactoryAddress, forwarderImplementationAddress, index ); possibleConsolidationAddresses.push(forwarderAddress);

mullapudipruthvik · 2025-08-08T04:57:48Z

modules/sdk-coin-eth/test/unit/eth.ts

+        basecoin = bitgo.coin('hteth') as Hteth;
+      });
+
+      it('should generate an unsigned consolidation', async function () {


add a test case for v5 and v6 separately

We don't need to write separate test cases since all the values are provided and based on the which wallet version we are assuming, we are using the relevant values and generating address for consolidation

in that case this would execute for v6 wallet as well right ?
const forwarderAddress = this.generateForwarderAddress( params.walletContractAddress, params.bitgoFeeAddress, forwarderFactoryAddress, forwarderImplementationAddress, index ); possibleConsolidationAddresses.push(forwarderAddress);

yes, this will run in both the cases since we have no means of verifying which wallet version is this

why are you sure whether this would throw an error for V6 wallet?
better to add a testcase to verify

ticket: WIN-5700

abhi-bitgo force-pushed the win-5700-eth-consolidation-recovery branch from 960e8af to 3b58e6a Compare July 25, 2025 13:29

abhi-bitgo changed the title ~~feat(eth): add recover consolidation for eth~~ feat(abstract-eth): add recover consolidation for eth Jul 25, 2025

abhi-bitgo force-pushed the win-5700-eth-consolidation-recovery branch 2 times, most recently from dfef449 to d50d402 Compare July 29, 2025 10:06

abhi-bitgo marked this pull request as ready for review July 29, 2025 12:28

abhi-bitgo requested review from a team as code owners July 29, 2025 12:28

mullapudipruthvik requested a review from Copilot August 4, 2025 06:46

Copilot AI reviewed Aug 4, 2025

View reviewed changes

mullapudipruthvik reviewed Aug 4, 2025

View reviewed changes

mullapudipruthvik requested changes Aug 4, 2025

View reviewed changes

abhi-bitgo force-pushed the win-5700-eth-consolidation-recovery branch 4 times, most recently from ce56893 to b0e63e4 Compare August 7, 2025 13:36

abhi-bitgo requested a review from mullapudipruthvik August 8, 2025 03:27

mullapudipruthvik reviewed Aug 8, 2025

View reviewed changes

abhi-bitgo requested a review from mullapudipruthvik August 11, 2025 04:06

mullapudipruthvik requested a review from a team August 12, 2025 07:50

abhi-bitgo force-pushed the win-5700-eth-consolidation-recovery branch from 383997d to aabf004 Compare August 13, 2025 06:02

feat(abstract-eth): add recover consolidation for eth

596e192

ticket: WIN-5700

abhi-bitgo force-pushed the win-5700-eth-consolidation-recovery branch from aabf004 to 596e192 Compare August 13, 2025 06:03

	console.log(`Failed to generate forwarder address: ${e.message}`);
	debug(`Failed to generate forwarder address: ${e instanceof Error ? e.message : e}`);

	console.log(`Failed to generate derived address: ${e}`);
	debug(`Failed to generate derived address: ${e}`);

	bitgoKey: '',
	...(params.bitgoKey !== undefined ? { bitgoKey: params.bitgoKey } : {}),

feat(abstract-eth): add recover consolidation for eth #6550

Are you sure you want to change the base?

feat(abstract-eth): add recover consolidation for eth #6550

Conversation

abhi-bitgo commented Jul 25, 2025

Uh oh!

mullapudipruthvik commented Aug 1, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mullapudipruthvik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mullapudipruthvik Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mullapudipruthvik Aug 13, 2025 •

edited

Loading