Proposal of refactor for BridgeSupportProcessFundsMigrationTest #3436

julianlen · 2026-01-16T20:25:56Z

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
Tests for the changes have been added (for bug fixes / features)
Requires Activation Code (Hard Fork)

Other information:

github-actions · 2026-01-16T20:26:13Z

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

marcos-iov

The test setup looks in general. I think we can improve a couple of things but not using mocks and not passing unnecessary values to the different builders

marcos-iov · 2026-01-19T13:53:57Z

rskj-core/src/test/java/co/rsk/peg/BridgeSupportProcessFundsMigrationTest.java


+    @BeforeEach
+    void setUp() {
+        feePerKbSupport = mock(FeePerKbSupport.class);


Any chance we could use a real object and not mock this? Like this

rskj/rskj-core/src/test/java/co/rsk/peg/feeperkb/FeePerKbIT.java

Line 36 in 5b8aa47

feePerKbSupport = new FeePerKbSupportImpl(feePerKbConstants, feePerKbStorageProvider);

marcos-iov · 2026-01-19T13:56:21Z

rskj-core/src/test/java/co/rsk/peg/BridgeSupportProcessFundsMigrationTest.java

+    void setUp() {
+        feePerKbSupport = mock(FeePerKbSupport.class);
+
+        bridgeEventLogger = mock(BridgeEventLogger.class);


Do we need this? looks like it's only used to pass it to the BridgeSupportBuilder but that's not really needed

marcos-iov · 2026-01-19T13:57:21Z

rskj-core/src/test/java/co/rsk/peg/BridgeSupportProcessFundsMigrationTest.java

+        // Arrange
+        long newFederationCreationBlockNumber = 5L;
+        Federation newFederation = standardMultiSigFederationBuilder
+            .withCreationTime(Instant.EPOCH)


Need to pass this? Does it make any difference? Let's try to kkep it as simple as possible

marcos-iov · 2026-01-19T14:09:49Z

rskj-core/src/test/java/co/rsk/peg/BridgeSupportProcessFundsMigrationTest.java

+        }
+    }
+
+    private List<UTXO> generateRetiringFederationUTXOs(Address retiringFederationAddress, int numberOfUtxos, int valuePerOutput) {


This is actually doing 2 things and that's not good. It creates and returns the UTXOs but also sets them in the storage which is actually a hidden side effect of the method

marcos-iov · 2026-01-19T14:10:44Z

rskj-core/src/test/java/co/rsk/peg/BridgeSupportProcessFundsMigrationTest.java

+        return block;
+    }
+
+    private void callUpdateCollections(BridgeSupport bridgeSupport) throws IOException {


We should consider having this in a util class. Not the actual call, but the creation of the updateCollections transaction probably.

Could be in BridgeSupportTestUtil I guess, or feel free to propose another idea

marcos-iov · 2026-01-19T14:11:58Z

rskj-core/src/test/java/co/rsk/peg/BridgeSupportProcessFundsMigrationTest.java

+        bridgeSupport.updateCollections(updateCollectionsTx);
+    }
+
+    private static List<UTXO> getOldFederationUtxos(int numberOfUtxos, int valuePerOutput, Address oldFederationAddress) {


This just creates UTXOs, doesn't matter if it's for a old/new or even for a federation.

Probably best to have a util to create UTXOs, I think there is already one at least in RskTestUtils class. Or maybe we should have a UTXOBuilder class?

In BitcoinTestUtils there is a createUTXOs method. However, it doesn't receive the amount as a parameter. I can create another method in BitcoinTestUtils that also receives the amount. I'm not sure a builder is necessary in this case.

Created another public static List<UTXO> createUTXOs(int numberOfUtxos, Coin value, Address receiver) method in BitcionTestUtils

marcos-iov · 2026-01-19T14:12:35Z

rskj-core/src/test/java/co/rsk/peg/BridgeSupportProcessFundsMigrationTest.java

+        return oldFederationUtxos;
+    }
+
+    public static Repository createRepository() {


I think we already have this in some test utils classes

Actually it is repeated in 3 different classes.

(added to the document)

marcos-iov · 2026-01-19T14:13:59Z

rskj-core/src/test/java/co/rsk/peg/BridgeSupportProcessFundsMigrationTest.java

+    }
+
+    private Block getBlockInMigrationAge(long newFederationCreationBlockNumber) {
+        Block block = mock(Block.class);


There is a method createRskBlock in RskTestUtil, we could use that to avoid creating a mock here

marcos-iov · 2026-01-19T14:15:47Z

rskj-core/src/test/java/co/rsk/peg/BridgeSupportProcessFundsMigrationTest.java

+        // TODO(juli): // provider.setPegoutTxSigHash(pegoutTxSigHash.get()); <- idem
+    }
+
+    private Coin calculateUtxosTotalValue(List<UTXO> retiringFederationUtxos) {


Since you are just summing up the values of a UTXOs list, I wouldn't call the param retiringFederationUtxos. The method could receive any list of UTXOs regarding of where they come from

marcos-iov · 2026-01-19T14:18:49Z

rskj-core/src/test/java/co/rsk/peg/BridgeSupportProcessFundsMigrationTest.java

+        assertTheReleaseOutpointValuesAreCorrect(btcTransaction, numberOfUtxos, retiringFederationUtxos);
+
+        // TODO(juli): // provider.setPegoutTxSigHash(pegoutTxSigHash.get()); <- idem
+    }


assert that all available UTXOs were included in the migration?

When processing a migration request, BridgeSupport calls provider.setPegoutTxSigHash(pegoutTxSigHash.get()). However, the value stays in the pegoutTxSigHashes set, which is a private variable, until save() is called, which moves the values in the pegoutTxSigHashes to the Repository. Is this out of the scope?

added a way to check all the retiring federation utxos are included in the migration tx

…ectionTransaction to BridgeSupportTestUtil, deleted unnecessary variables in the BridgeSupportProcessFundsMigrationTest setup, created in BitcoinTestUtils a new method to create a list of UTXOs with a certain amount and receiver, used createRskBlock from RskTestUtil

Copilot

Pull request overview

This PR refactors the BridgeSupportProcessFundsMigrationTest test class by extracting common test utilities and consolidating duplicate code, improving test maintainability and reducing code duplication.

Changes:

Extracted common transaction building and UTXO creation utilities into shared helper classes
Added a new comprehensive test case for funds migration with proper setup
Removed duplicate utility methods across multiple test files

Reviewed changes

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

Show a summary per file

File	Description
BridgeSupportProcessFundsMigrationTest.java	Refactored test class with extracted utilities, added new test case, and included setup method with instance variables
BridgeSupportTestUtil.java	Added `buildUpdateCollectionsTx` utility method for transaction creation
BitcoinTestUtils.java	Added overloaded `createUTXOs` method accepting value and receiver parameters
BridgeSupportIT.java	Removed duplicate `createRepository` method, now using shared utility
FederationChangeIT.java	Updated to use shared `buildUpdateCollectionsTx` utility instead of local implementation

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-19T15:36:27Z

rskj-core/src/test/java/co/rsk/peg/BridgeSupportProcessFundsMigrationTest.java

        }

-        if (activations.isActive(ConsensusRule.RSKIP146)) {
+        if (activations.isActive(ConsensusRule.RSKIP146)) { // Papyrus


While 'Papyrus' appears to be a codename for RSKIP146, this comment lacks context for readers unfamiliar with the codebase. Consider expanding it to explain what Papyrus represents or the significance of this activation rule.

Suggested change

if (activations.isActive(ConsensusRule.RSKIP146)) { // Papyrus

if (activations.isActive(ConsensusRule.RSKIP146)) { // Papyrus (RSKIP146) network upgrade: enables logging and tracking of migrated pegouts after activation

Copilot · 2026-01-19T15:36:27Z

rskj-core/src/test/java/co/rsk/peg/BridgeSupportProcessFundsMigrationTest.java

            );

-            if (activations.isActive(ConsensusRule.RSKIP376)){
+            if (activations.isActive(ConsensusRule.RSKIP376)){ // Arrowhead


While 'Arrowhead' appears to be a codename for RSKIP376, this comment lacks context for readers unfamiliar with the codebase. Consider expanding it to explain what Arrowhead represents or the significance of this activation rule.

Suggested change

if (activations.isActive(ConsensusRule.RSKIP376)){ // Arrowhead

if (activations.isActive(ConsensusRule.RSKIP376)){ // Arrowhead (RSKIP376 upgrade): after activation, pegout BTC transactions must use version 2 instead of the legacy version

Copilot · 2026-01-19T15:36:28Z

rskj-core/src/test/java/co/rsk/peg/BridgeSupportProcessFundsMigrationTest.java

+
+        assertTheReleaseOutpointValuesAreCorrect(btcTransaction, numberOfUtxos, retiringFederationUtxos);
+
+        // TODO(juli): // provider.setPegoutTxSigHash(pegoutTxSigHash.get()); <- idem


This TODO comment contains unclear references ('juli', 'idem'). TODO comments should clearly describe what needs to be done, why it's needed, and provide enough context for any developer to understand and implement the task.

Suggested change

// TODO(juli): // provider.setPegoutTxSigHash(pegoutTxSigHash.get()); <- idem

// TODO: When pegout transaction sighash is stored for migrated funds, update this test to set the sighash on the provider and assert it is persisted correctly.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…at all the retiringFederation utxos are actually included as input in the migration tx

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

rskj-core/src/test/java/co/rsk/peg/BridgeSupportProcessFundsMigrationTest.java

sonarqubecloud · 2026-01-21T19:11:58Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

julia-zack · 2026-01-22T19:07:40Z

rskj-core/src/test/java/co/rsk/peg/BridgeSupportProcessFundsMigrationTest.java

+    }
+
+    @Test
+    void processFundsMigration_withRetiringGenesisFederationAndNewMultisigFederation_inMigrationAge_shouldMigrateFunds() throws IOException {


i think we should not use all activations for testing this scenario. For example, the releaseOutpointsValues assertion does not make sense in this case

julia-zack · 2026-01-22T19:11:58Z

rskj-core/src/test/java/co/rsk/peg/BridgeSupportTestUtil.java

        return new MutableRepository(new MutableTrieCache(new MutableTrieImpl(null, new Trie())));
    }

+    public static Transaction buildUpdateCollectionsTx(byte chainId) {


probably not the best to receive a byte as argument, can't we pass the Constants and get the chainId from them?

julia-zack · 2026-01-22T19:12:19Z

rskj-core/src/test/java/co/rsk/peg/BridgeSupportTestUtil.java

+        var rskTx = CallTransaction.createCallTransaction(nonce, gasPrice.longValue(),
+            gasLimit.longValue(), PrecompiledContracts.BRIDGE_ADDR, value,
+            Bridge.UPDATE_COLLECTIONS, chainId);
+        var randomKey = BtcECKey.fromPrivate(Hex.decode("45c5b07fc1a6f58892615b7c31dca6c96db58c4bbc538a6b8a22999aaa860c32"));


where does this priv key came from? can we use a random seed instead?

julia-zack · 2026-01-22T19:24:29Z

rskj-core/src/test/java/co/rsk/peg/BridgeSupportProcessFundsMigrationTest.java

+        Federation retiringFederation = FederationTestUtils.getGenesisFederation(federationConstants);
+        int numberOfUtxos = 1;
+        Coin valuePerOutput = Coin.valueOf(1_500_000);
+        Address retiringFederationAddress = retiringFederation.getAddress();
+
+        List<UTXO> retiringFederationUtxos = createUTXOs(numberOfUtxos, valuePerOutput, retiringFederationAddress);
+        byte[] serializeUTXOList = BridgeSerializationUtils.serializeUTXOList(retiringFederationUtxos);
+        bridgeStorageAccessor.saveToRepository(OLD_FEDERATION_BTC_UTXOS_KEY.getKey(), serializeUTXOList);
+
+        long newFederationCreationBlockNumber = 5L;
+        Federation newFederation = standardMultiSigFederationBuilder
+            .withCreationBlockNumber(newFederationCreationBlockNumber)
+            .withNetworkParameters(networkParams)
+            .build();
+
+        federationStorageProvider.setOldFederation(retiringFederation);
+        federationStorageProvider.setNewFederation(newFederation);
+
+        Block rskCurrentBlock = getBlockInMigrationAge(newFederationCreationBlockNumber);
+        FederationSupport federationSupport = FederationSupportBuilder.builder()
+            .withFederationConstants(federationConstants)
+            .withFederationStorageProvider(federationStorageProvider)
+            .withRskExecutionBlock(rskCurrentBlock)
+            .withActivations(activations)
+            .build();
+
+        BridgeSupport bridgeSupport = BridgeSupportBuilder.builder()
+            .withBridgeConstants(bridgeMainnetConstants)
+            .withProvider(bridgeStorageProvider)
+            .withExecutionBlock(rskCurrentBlock)
+            .withActivations(activations)
+            .withFeePerKbSupport(feePerKbSupport)
+            .withFederationSupport(federationSupport)
+            .build();


not needed yet, but we should probably move all this to a setup function

totally agree

julia-zack · 2026-01-22T19:27:15Z

rskj-core/src/test/java/co/rsk/peg/BridgeSupportProcessFundsMigrationTest.java

+        }
+    }
+
+    private Block getBlockInMigrationAge(long newFederationCreationBlockNumber) {


can we move this to a utility file as a "getBlockWithHeight" method?

julianlen requested a review from a team as a code owner January 16, 2026 20:25

julianlen marked this pull request as draft January 16, 2026 20:26

julianlen marked this pull request as ready for review January 19, 2026 14:18

marcos-iov reviewed Jan 19, 2026

View reviewed changes

julianlen force-pushed the refactor-BridgeSupportProcessFundsMigrationTest branch from 5f6d1f0 to 137059b Compare January 19, 2026 15:24

julianlen added 2 commits January 19, 2026 12:25

Created alternative to test BridgeSupportProcessFundsMigration

41f6740

julianlen force-pushed the refactor-BridgeSupportProcessFundsMigrationTest branch from 137059b to 9bada07 Compare January 19, 2026 15:25

julianlen requested a review from Copilot January 19, 2026 15:35

Copilot AI reviewed Jan 19, 2026

View reviewed changes

julianlen requested a review from Copilot January 19, 2026 15:38

Copilot AI reviewed Jan 19, 2026

View reviewed changes

julianlen added 2 commits January 19, 2026 13:40

oldFederation is now called retiringFederation, also added a check th…

57edd9c

…at all the retiringFederation utxos are actually included as input in the migration tx

cleaning the code for review

e71875c

julianlen requested a review from Copilot January 21, 2026 18:55

Copilot AI reviewed Jan 21, 2026

View reviewed changes

rskj-core/src/test/java/co/rsk/peg/BridgeSupportProcessFundsMigrationTest.java Show resolved Hide resolved

julia-zack reviewed Jan 22, 2026

View reviewed changes

marcos-iov marked this pull request as draft January 27, 2026 13:11

	if (activations.isActive(ConsensusRule.RSKIP146)) { // Papyrus
	if (activations.isActive(ConsensusRule.RSKIP146)) { // Papyrus (RSKIP146) network upgrade: enables logging and tracking of migrated pegouts after activation


		assertTheReleaseOutpointValuesAreCorrect(btcTransaction, numberOfUtxos, retiringFederationUtxos);

		// TODO(juli): // provider.setPegoutTxSigHash(pegoutTxSigHash.get()); <- idem

	// TODO(juli): // provider.setPegoutTxSigHash(pegoutTxSigHash.get()); <- idem
	// TODO: When pegout transaction sighash is stored for migrated funds, update this test to set the sighash on the provider and assert it is persisted correctly.

Proposal of refactor for BridgeSupportProcessFundsMigrationTest #3436

Are you sure you want to change the base?

Proposal of refactor for BridgeSupportProcessFundsMigrationTest #3436

Conversation

julianlen commented Jan 16, 2026

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

github-actions bot commented Jan 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dependency Review

Scanned Files

Uh oh!

marcos-iov 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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

github-actions bot commented Jan 16, 2026 •

edited

Loading