Legacy peg in happy path #2886

julianlen · 2024-12-10T14:14:46Z

Description

Added RegisterBtcTransactionIT, a new integration tests suit with the test whenRegisterALegacyBtcTransactionTheBridgeShouldRegisterTheNewUtxoAndTransferTheRbtcBalance.

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 · 2024-12-10T14:15:07Z

Dependency Review

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

OpenSSF Scorecard

Package	Version	Score	Details

Scanned Manifest Files

apancorb

Looks really good overall. Added a bunch of comments about semantics we tend to use for writing unit tests.

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

apancorb · 2024-12-11T14:32:34Z

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

+    private final BridgeSupportBuilder bridgeSupportBuilder = BridgeSupportBuilder.builder();
+
+    @Test
+    void whenRegisterALegacyBtcTransactionTheBridgeShouldRegisterTheNewUtxoAndTransferTheRbtcBalance() throws BlockStoreException, AddressFormatException, IOException, BridgeIllegalArgumentException {


The pattern we tend to follow is methodName_when..._should.... Since this is an integration test is fine to leave as blank the methodName part.

Note also that there is really not a need to declare all the types of exceptions since this is a test. If a test expects an exception to be thrown it should be asserted. So a simple Exception declaration will suffice.

Suggested change

void whenRegisterALegacyBtcTransactionTheBridgeShouldRegisterTheNewUtxoAndTransferTheRbtcBalance() throws BlockStoreException, AddressFormatException, IOException, BridgeIllegalArgumentException {

void whenRegisteringALegacyBtcTransaction_shouldRegisterTheNewUtxoAndTransferTheRbtcBalance() throws Exception {

Surrounding the methods with try/catch and fail the test in case of an exception, is it a valid approach?

What I mean is that if the test expects an exception thrown, then we can assert it as assertThrows(Exception.class, () -> methodThatThrowsException());. But here since it is not expected we can declare it in the method signature throws Exception. This reduces boilerplate code since we know this test will not throw an exception since it is not intended.

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

nathanieliov · 2024-12-12T03:57:48Z

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

+    private final BridgeSupportBuilder bridgeSupportBuilder = BridgeSupportBuilder.builder();
+
+    @Test
+    void whenRegisterALegacyBtcTransaction_shouldRegisterTheNewUtxoAndTransferTheRbtcBalance() {


Suggested change

void whenRegisterALegacyBtcTransaction_shouldRegisterTheNewUtxoAndTransferTheRbtcBalance() {

void whenValidLegacyPegin_shouldRegisterNewUtxoAndTransferTheRbtcBalance() {

We usually use this naming convention "$method_$condition_$expectedResult". I assume, in this case, we don't need the method_name prefix since it is already expressed in the class name, but I think we can keep the rest part as we already do following the existing naming convention used in other tests

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

nathanieliov · 2024-12-12T04:02:23Z

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

+        FederationSupport federationSupport = getFederationSupport(federationStorageProvider, activationConfig, bridgeConstants.getFederationConstants());
+
+        BtcECKey btcPublicKey = new BtcECKey();
+        Coin btcTransferred = Coin.COIN;


Let's use a more common/realistic value, like minimumPegin, wdyt?

I thought COIN was normally used as regular value

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

Line 544 in 4fbdf69

TransactionOutput output = new TransactionOutput(btcRegTestParams, t, Coin.COIN, new BtcECKey().toAddress(btcRegTestParams));

Well, on Mainnet, I don't think many users will send 1 BTC. The most common value I think, will be close to the minimum pegin value.

Probably a good idea to use the min peg-in value since it makes the test adapt in case that value ever changes. And we are also testing a corner case

perfecto, changed already!

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

nathanieliov · 2024-12-12T04:05:19Z

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

+
+        BridgeStorageProvider bridgeStorageProvider = new BridgeStorageProvider(track, PrecompiledContracts.BRIDGE_ADDR, bridgeConstants.getBtcParams(), activationConfig);
+        BtcBlockStoreWithCache.Factory btcBlockStoreFactory = new RepositoryBtcBlockStoreWithCache.Factory(bridgeConstants.getBtcParams(), 100, 100);
+        BtcBlockStoreWithCache btcBlockStoreWithCache = btcBlockStoreFactory.newInstance(track, bridgeConstants, bridgeStorageProvider, activationConfig);


Why do not use the default RepositoryBtcBlockStoreWithCache constructor?

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

marcos-iov

Looking good, well done. Left some ideas and suggestions

marcos-iov · 2024-12-13T17:14:08Z

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

@@ -52,7 +56,7 @@ public static void recreateChainFromPmt(
        btcBlockStoreWithCache.setChainHead(storedChainHeadBlock);
    }

-    private static BtcBlock createBtcBlockWithPmt(PartialMerkleTree pmt, NetworkParameters networkParameters) {
+    public static BtcBlock createBtcBlockWithPmt(PartialMerkleTree pmt, NetworkParameters networkParameters) {


Should this be part of BitcoinTestUtils class maybe?

marcos-iov · 2024-12-13T17:19:52Z

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

+        FederationStorageProvider federationStorageProvider = getFederationStorageProvider(track, federation);
+        FederationSupport federationSupport = getFederationSupport(federationStorageProvider, activations, bridgeConstants.getFederationConstants());
+
+        BtcECKey btcPublicKey = new BtcECKey();


Let's use BitcoinTestUtils class to create keys using a seed, that way the test is deterministic

marcos-iov · 2024-12-13T17:20:43Z

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

+        FederationSupport federationSupport = getFederationSupport(federationStorageProvider, activationConfig, bridgeConstants.getFederationConstants());
+
+        BtcECKey btcPublicKey = new BtcECKey();
+        Coin btcTransferred = Coin.COIN;


Probably a good idea to use the min peg-in value since it makes the test adapt in case that value ever changes. And we are also testing a corner case

marcos-iov · 2024-12-13T17:45:41Z

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

+        Coin btcTransferred = Coin.COIN;
+        BtcTransaction bitcoinTransaction = createPegInTransaction(federationSupport.getActiveFederation().getAddress(), btcTransferred, btcPublicKey);
+        TransactionOutput output = bitcoinTransaction.getOutput(0);
+        List<UTXO> expectedFederationUtxos = Collections.singletonList(getUtxo(bitcoinTransaction, output));


Something very very similar is already done in BridgeSupport::saveNewUTXOs.

Perhaps this should be a util method in BitcoinUtils class? To be used both in tests and in the Bridge code.

Not to do it as part of this PR, but we could create a new ticket for that

marcos-iov · 2024-12-13T17:47:45Z

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

+
+        BtcECKey btcPublicKey = new BtcECKey();
+        Coin btcTransferred = Coin.COIN;
+        BtcTransaction bitcoinTransaction = createPegInTransaction(federationSupport.getActiveFederation().getAddress(), btcTransferred, btcPublicKey);


Perhaps we could consider creating a PeginTransactionBuilder? What do you think? Would it be worth it?

With some methods like addInput(prevHash, outputIndex) and addOutput(address, value). This could then scale to considering different address types maybe so we could start setting up new test cases easily.

Again, not to apply in this PR but something to consider as part of this work

marcos-iov · 2024-12-13T17:48:30Z

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

+    }
+
+    private static FeePerKbSupport getFeePerKbSupport() {
+        FeePerKbSupport feePerKbSupport = mock(FeePerKbSupport.class);


Is there any chance we could create the actual object and not a mock?

marcos-iov · 2024-12-13T17:49:30Z

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

+        return feePerKbSupport;
+    }
+
+    private static Block getRskExecutionBlock() {


Maybe this could go in BridgeSupportTestUtil? Seems like it might be useful in other places. Or in RskTestUtil maybe if we plan to use this for other things than just the Bridge

So I'll move it to BridgeSupportTestUtil. If in the future we need it for something but the Bridge, we can move it to RskTestUtil to make it more "general"

marcos-iov · 2024-12-13T17:50:24Z

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

+        assertTrue(heightIfBtcTxHashIsAlreadyProcessed.isPresent());
+        assertEquals(rskExecutionBlock.getNumber(), heightIfBtcTxHashIsAlreadyProcessed.get());
+        assertEquals(expectedFederationUtxos, federationSupport.getActiveFederationBtcUTXOs());
+        assertEquals(expectedReceiverBalance, repository.getBalance(receiver));


Could we also assert that pegin_btc event was emitted? With the expected values

Assert that the transaction was marked as processed

Regarding the assert whether the tx was marked as processed I have

Optional<Long> heightIfBtcTxHashIsAlreadyProcessed = bridgeStorageProvider.getHeightIfBtcTxhashIsAlreadyProcessed(bitcoinTransaction.getHash()); assertTrue(heightIfBtcTxHashIsAlreadyProcessed.isPresent()); assertEquals(rskExecutionBlock.getNumber(), heightIfBtcTxHashIsAlreadyProcessed.get());

done the assertion of the event

marcos-iov · 2024-12-13T17:52:02Z

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

+
+        return btcTx;
+    }
+}


Remember to leave a white space at the end of each file

…t parameters

…ministic

apancorb

Nice job :) . Watch out for the sonar complaints.

apancorb · 2024-12-16T10:42:45Z

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

+import org.ethereum.vm.PrecompiledContracts;
+import org.junit.jupiter.api.Test;
+import java.util.*;
+


nit: remove extra blank line

apancorb · 2024-12-16T10:43:52Z

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

+        );
+    }
+
+    private BridgeSupport getBridgeSupport(BridgeEventLoggerImpl bridgeEventLogger, BridgeStorageProvider bridgeStorageProvider, ActivationConfig.ForBlock activationsBeforeForks, FederationSupport federationSupport, FeePerKbSupport feePerKbSupport, Block rskExecutionBlock, BtcBlockStoreWithCache.Factory btcBlockStoreFactory, Repository repository, BtcLockSenderProvider btcLockSenderProvider) {


In the future as discussed in another PR, lets avoid this type of methods and use the builder directly

nathanieliov · 2024-12-16T17:43:16Z

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

-    private FederationStorageProvider createFederationStorageProvider(Repository repository) {
-        StorageAccessor bridgeStorageAccessor = new BridgeStorageAccessorImpl(repository);
-        return new FederationStorageProviderImpl(bridgeStorageAccessor);
-    }
 }


Missing an empty line in this file for git management

nathanieliov · 2024-12-16T17:47:53Z

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

+import java.util.*;
+
+
+public class RegisterBtcTransactionIT {


Remove the public modifier to comply with Sonar rules.

nathanieliov · 2024-12-16T18:08:30Z

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

+        verify(bridgeEventLogger, times(1)).logPeginBtc(receiver, bitcoinTransaction, btcTransferred, 0);
+    }
+
+    private static UTXO getUtxo(BtcTransaction bitcoinTransaction, TransactionOutput output) {


What do you think of naming this method utxoOf following the Java naming convention used for static creational patterns? See:

List.of Set.of LocalDate.of

We've been using this naming convention in some places. For example:

rskj/rskj-core/src/main/java/co/rsk/peg/bitcoin/P2shErpRedeemScriptBuilder.java

Line 22 in 08b28eb

public Script of(

sonarqubecloud · 2024-12-16T18:42:10Z

Quality Gate passed

Issues
0 New issues
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

* feat: first happy path for a peg in * fix: declaration of utxo * fix: black space at the end of the file * fix: assert that the event pegin_btc has been emitted with the correct parameters * feat: replaced mocked feePerKeb constant for an actual instance * fix: now the peg in uses the minimum pegin tx value * fix: moved the method getRskExecutionBlock to BridgeSupoprtTestUtil * fix: using BitcoinTestUtils to make the btcPublicKey generation deterministic * fix: erased extra line * fix: added in BridgeSupportIT an extra line in the end * fix: removed the public modifier in the RegisterBTCTransactionIT * fix: renamed getUtxo to utxoOf

julianlen requested a review from a team as a code owner December 10, 2024 14:14

apancorb reviewed Dec 11, 2024

View reviewed changes

julianlen requested a review from apancorb December 11, 2024 19:50

nathanieliov reviewed Dec 12, 2024

View reviewed changes

feat: first happy path for a peg in

a1d3e08

julianlen force-pushed the pegin-it-initialTest branch from 21f7774 to a1d3e08 Compare December 12, 2024 20:14

fix: declaration of utxo

cfa3e01

marcos-iov reviewed Dec 13, 2024

View reviewed changes

julianlen added 6 commits December 13, 2024 15:21

fix: black space at the end of the file

3a4f9f9

fix: assert that the event pegin_btc has been emitted with the correc…

202a4ae

…t parameters

feat: replaced mocked feePerKeb constant for an actual instance

f60be9b

fix: now the peg in uses the minimum pegin tx value

d2f8a90

fix: moved the method getRskExecutionBlock to BridgeSupoprtTestUtil

7a86a5d

fix: using BitcoinTestUtils to make the btcPublicKey generation deter…

c4825b2

…ministic

apancorb approved these changes Dec 16, 2024

View reviewed changes

nathanieliov approved these changes Dec 16, 2024

View reviewed changes

julianlen added 4 commits December 16, 2024 15:19

fix: erased extra line

11c36df

fix: added in BridgeSupportIT an extra line in the end

141ef7e

fix: removed the public modifier in the RegisterBTCTransactionIT

e684032

fix: renamed getUtxo to utxoOf

097cba5

julianlen merged commit db3f6bc into pegin-tests-integration Dec 16, 2024
9 checks passed

julianlen deleted the pegin-it-initialTest branch December 16, 2024 18:49

aeidelman added this to the Lovell 7.0.0 milestone Jan 28, 2025

	void whenRegisterALegacyBtcTransactionTheBridgeShouldRegisterTheNewUtxoAndTransferTheRbtcBalance() throws BlockStoreException, AddressFormatException, IOException, BridgeIllegalArgumentException {
	void whenRegisteringALegacyBtcTransaction_shouldRegisterTheNewUtxoAndTransferTheRbtcBalance() throws Exception {

	void whenRegisterALegacyBtcTransaction_shouldRegisterTheNewUtxoAndTransferTheRbtcBalance() {
	void whenValidLegacyPegin_shouldRegisterNewUtxoAndTransferTheRbtcBalance() {

Legacy peg in happy path #2886

Legacy peg in happy path #2886

Conversation

julianlen commented Dec 10, 2024

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

github-actions bot commented Dec 10, 2024 • edited Loading

Dependency Review

OpenSSF Scorecard

Scanned Manifest Files

apancorb 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

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

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

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

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

apancorb left a comment • 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

sonarqubecloud bot commented Dec 16, 2024

Quality Gate passed

github-actions bot commented Dec 10, 2024 •

edited

Loading

apancorb left a comment •

edited

Loading