01-04-54-post-iris-call-receive-headers / 01-04-55-post-iris-call-receive-header #26

jpikora-iov · 2023-12-19T12:51:47Z

first part refactor of 01-04-54-post-iris-call-receive-headers 01_04_54-post_iris_call_receive_headers.js

marcos-iov

Changes look good but if the plan is to run the same test 3 times we should encapsulate the logic in a single file to avoid repeating code everywhere. I can help you with this

marcos-iov · 2023-12-19T21:51:11Z

package-lock.json

@@ -1,7 +1,7 @@
 {
  "name": "rootstock-integration-tests",
  "version": "1.0.0",
-  "lockfileVersion": 3,
+  "lockfileVersion": 2,


Make sure you are using node v18

Using node v20 now.

jeremy-then

Left a couple of comments. Rebase with the latest changes on main. And run npm i with nodejs version 20.

jeremy-then · 2023-12-19T22:51:17Z

tests/01_04_54-post_iris_call_receive_headers.js

+      const bridge = getBridge(rskTxHelper.getClient(), await getLatestActiveForkName());
+      const blockNumberInitial = await bridge.methods.getBtcBlockchainBestChainHeight().call();
+      const cowAddress = await rskTxHelper.newAccountWithSeed('cow');
+      const blockHash = await btcTxHelper.mine();


I know we have things like this in many places, but it's a bit confusing. Let's start fixing that.

The await btcTxHelper.mine(); call returns an array of btc block hashes, the ones that were mined. In this case, only one block was mined. But still, it returns an array. So let's called that blockHashes to make it clear.

jeremy-then · 2023-12-19T22:59:45Z

tests/05_03_54-last_fork_active_call_receive_headers.js

+const { getRskTransactionHelpers } = require('../lib/rsk-tx-helper-provider');
+const { getBtcClient } = require('../lib/btc-client-provider');
+const { getBridge, getLatestActiveForkName } = require('../lib/precompiled-abi-forks-util');
+const { activateFork, sendTxWithCheck } = require('../lib/rsk-utils');


activateFork is not being used here.

jpikora-iov · 2024-01-03T12:35:09Z

pipeline:run

jeremy-then · 2024-01-03T13:55:36Z

pipeline:run

jeremy-then

LGTM.

marcos-iov · 2024-01-03T18:37:29Z

tests/01_04_54-post_iris_call_receive_headers.js

-        }
-    });
-});
+twoWpTests.execute('Calling receiveHeaders after iris300', () => Runners.hosts.federate.host);


Remember to leave a blank line at the end of each file, check the pther files also

marcos-iov · 2024-01-03T18:38:01Z

tests/01_04_54-post_iris_call_receive_headers.js

@@ -1,47 +1,3 @@
-const expect = require('chai').expect;
+const twoWpTests = require('../lib/tests/post_iris_call_receive_headers');


Suggested change

const twoWpTests = require('../lib/tests/post_iris_call_receive_headers');

const receiveHeadersTests = require('../lib/tests/post_iris_call_receive_headers');

Check the same on the remaining files

marcos-iov · 2024-01-03T18:41:55Z

tests/03_03_54-last_fork_active_call_receive_headers.js

@@ -0,0 +1,3 @@
+const twoWpTests = require('../lib/tests/post_iris_call_receive_headers');
+
+twoWpTests.execute('Calling receiveHeaders after arrowhead600', () => Runners.hosts.federate.host);


Suggested change

twoWpTests.execute('Calling receiveHeaders after arrowhead600', () => Runners.hosts.federate.host);

twoWpTests.execute('Calling receiveHeaders after last fork is active', () => Runners.hosts.federate.host);

If we put here after arrowhead then we'll need to update this every time a new rskj version is released

Check the other files also

Also the file naming seems a bit confusing. 03 prefix means that the latest fork is active, right? So no need to call each file last_fork_active.... And what's the logic for the numbers following 03_? Some are 01, 03, 50, etc

i guess we are just following the line / list to keep the order of execution... @ed-iov ?

03_02_01-last_fork_active_powpeg_redeem_script.js
03_03_54-last_fork_active_call_receive_headers.js
03_03_55-last_fork_active_call_receive_header.js
04_00_01-key_control_initial_federation.js
04_00_02-fedchange.js
04_00_03-key_control_second_federation.js
04_00_04-2wp_after_fedchange.js
04_00_05-fastbridge_operation_with_rejection.js
04_00_06-fastbridge_after_fed_change.js
05_00_01-fee_per_kb.js
05_02_01-last_fork_active_powpeg_redeem_script.js
05_03_54-last_fork_active_call_receive_headers.js
05_03_55-last_fork_active_call_receive_header.js

the 54 / 55 comes from the original name. so i believe these can be skipped or replaced with 00...

marcos-iov · 2024-01-03T19:29:47Z

lib/tests/post_iris_call_receive_header.js

+            const blockHeader = await btcTxHelper.getBlockHeader(blockHashes[0], false);
+            const blockchainInitialHeigth = await bridge.methods.getBtcBlockchainBestChainHeight().call();
+
+            await sendTxWithCheck(


Can we put the method call and the callback in the own variables? It will make it easier to read

marcos-iov · 2024-01-03T19:30:14Z

lib/tests/post_iris_call_receive_header.js

+                rskTxHelper,
+                bridge.methods.receiveHeader(ensure0x(blockHeader)),
+                cowAddress,
+                (result) => { expect(Number(result)).to.be.equal(0) }


The expected result 0 should also probably be in it's own constant. It will make it much clearer to understand

marcos-iov · 2024-01-03T19:31:55Z

lib/tests/post_iris_call_receive_header.js

+            const blockHashes = await btcTxHelper.mine();
+            const blockHeader = await btcTxHelper.getBlockHeader(blockHashes[0], false);
+            const result = await bridge.methods.receiveHeader(ensure0x(blockHeader)).call();
+            expect(result).to.be.equal('-1');


Same with this value, perhaps we could have a file with receive header response codes

i would probably put just first 2 in the test file it's self...
why keep the rest if we don't use them?

const HEADER_RECEIVED = 0
const RECEIVE_HEADER_CALLED_TOO_SOON = -1;
const RECEIVE_HEADER_BLOCK_TOO_OLD = -2;
const RECEIVE_HEADER_CANT_FOUND_PREVIOUS_BLOCK = -3;
const RECEIVE_HEADER_BLOCK_PREVIOUSLY_SAVED = -4;
const RECEIVE_HEADER_UNEXPECTED_EXCEPTION = -99;

marcos-iov · 2024-01-03T19:33:04Z

lib/tests/post_iris_call_receive_header.js

+    await activateFork(Runners.common.forks[forkName]);
+};
+
+const execute = (description, getRskHost) => {


Not part of this ticket but I guess we should consider adding new test cases for the different error codes this method can return.

cc @ed-iov

marcos-iov · 2024-01-03T19:46:13Z

lib/tests/post_iris_call_receive_headers.js

+        const bridge = getBridge(rskTxHelper.getClient(), await getLatestActiveForkName());
+        const blockNumberInitial = await bridge.methods.getBtcBlockchainBestChainHeight().call();
+        const cowAddress = await rskTxHelper.newAccountWithSeed('cow');
+        const blockHashes = await btcTxHelper.mine();


Should we also all call updateBridge here as in receiveHeader test to make sure that the Bridge is up to date and that we a re informing a valid block?

not sure, it was in the original test for receiveHeader but not int the receiveHeaders one... @jeremy-then ??

jeremy-then

LGTM.

jeremy-then

LGTM.

jeremy-then

LGTM.

marcos-iov

Looks good now, just a minor comment. I'm not sure about adding last_fork_active prefix to all files though, what's the purpose? I thought that the number prefix 03, 04, 05 already indicates that the last fork is active, right?

marcos-iov · 2024-01-05T14:21:05Z

lib/tests/call_receive_header.js

@@ -9,6 +9,9 @@ let rskTxHelper;
 let btcTxHelper;
 let bridge;

+const HEADER_RECEIVED_OK = 0;
+const RECEIVE_HEADER_CALLED_TOO_SOON = "-1";


why string?

marcos-iov · 2024-01-05T14:22:16Z

lib/tests/call_receive_header.js

+            const blockHashes = await btcTxHelper.mine();
+            const blockHeader = await btcTxHelper.getBlockHeader(blockHashes[0], false);
+            const result = await bridge.methods.receiveHeader(ensure0x(blockHeader)).call();
+            expect(result).to.be.equal(RECEIVE_HEADER_CALLED_TOO_SOON);


Suggested change

expect(result).to.be.equal(RECEIVE_HEADER_CALLED_TOO_SOON);

expect(Number(result)).to.be.equal(RECEIVE_HEADER_CALLED_TOO_SOON);

That should do it

sonarqubecloud · 2024-01-05T16:23:24Z

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

marcos-iov · 2024-01-05T17:05:48Z

pipeline:run

jpikora-iov · 2024-01-08T12:02:47Z

pipeline:run

jeremy-then

LGTM.

jpikora-iov requested a review from a team as a code owner December 19, 2023 12:51

marcos-iov requested changes Dec 19, 2023

View reviewed changes

jeremy-then reviewed Dec 19, 2023

View reviewed changes

Jan Pikora and others added 6 commits January 3, 2024 01:52

refactor/01-04-54-post-iris-call-receive-headers

2b2c409

helpers declaration fix

e2cecf1

forgotten comments

1061f28

refactor/01-04-54-post-iris-call-receive-headers

0c30e59

getBlockHeader() part, test encapsulation

145f475

Updating bridge before running receiveHeader test

f325463

jeremy-then force-pushed the refactor/01-04-54-post-iris-call-receive-headers branch from ddacf09 to f325463 Compare January 3, 2024 05:53

Updates to node v20

74de4a0

Using the last fed after the fed change.

c920d95

jpikora-iov changed the title ~~WIP - 01-04-54-post-iris-call-receive-headers / 01-04-55-post-iris-call-receive-header~~ 01-04-54-post-iris-call-receive-headers / 01-04-55-post-iris-call-receive-header Jan 3, 2024

jeremy-then previously approved these changes Jan 3, 2024

View reviewed changes

marcos-iov reviewed Jan 3, 2024

View reviewed changes

addressing review comments

ff3cd61

jpikora-iov dismissed jeremy-then’s stale review via ff3cd61 January 4, 2024 18:50

jeremy-then approved these changes Jan 5, 2024

View reviewed changes

jeremy-then previously approved these changes Jan 5, 2024

View reviewed changes

marcos-iov reviewed Jan 5, 2024

View reviewed changes

file names updates

6a7f4da

jpikora-iov dismissed jeremy-then’s stale review via 6a7f4da January 5, 2024 16:22

marcos-iov approved these changes Jan 8, 2024

View reviewed changes

jeremy-then approved these changes Jan 8, 2024

View reviewed changes

jeremy-then merged commit d9d7d3b into main Jan 8, 2024
4 checks passed

jeremy-then deleted the refactor/01-04-54-post-iris-call-receive-headers branch January 8, 2024 13:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

01-04-54-post-iris-call-receive-headers / 01-04-55-post-iris-call-receive-header #26

01-04-54-post-iris-call-receive-headers / 01-04-55-post-iris-call-receive-header #26

jpikora-iov commented Dec 19, 2023

marcos-iov left a comment

marcos-iov Dec 19, 2023

jeremy-then Jan 3, 2024

jeremy-then left a comment

jeremy-then Dec 19, 2023

jeremy-then Dec 19, 2023

jpikora-iov commented Jan 3, 2024

jeremy-then commented Jan 3, 2024

jeremy-then left a comment

marcos-iov Jan 3, 2024

marcos-iov Jan 3, 2024

marcos-iov Jan 3, 2024

marcos-iov Jan 3, 2024

marcos-iov Jan 3, 2024

marcos-iov Jan 3, 2024

jpikora-iov Jan 4, 2024

marcos-iov Jan 3, 2024

marcos-iov Jan 3, 2024

marcos-iov Jan 3, 2024

jpikora-iov Jan 4, 2024

marcos-iov Jan 3, 2024

marcos-iov Jan 3, 2024

jpikora-iov Jan 4, 2024

jeremy-then left a comment

jeremy-then left a comment

jeremy-then left a comment

marcos-iov left a comment

marcos-iov Jan 5, 2024

marcos-iov Jan 5, 2024

sonarqubecloud bot commented Jan 5, 2024

marcos-iov commented Jan 5, 2024

jpikora-iov commented Jan 8, 2024

jeremy-then left a comment

		@@ -1,47 +1,3 @@
		const expect = require('chai').expect;
		const twoWpTests = require('../lib/tests/post_iris_call_receive_headers');

	const twoWpTests = require('../lib/tests/post_iris_call_receive_headers');
	const receiveHeadersTests = require('../lib/tests/post_iris_call_receive_headers');

		@@ -0,0 +1,3 @@
		const twoWpTests = require('../lib/tests/post_iris_call_receive_headers');

		twoWpTests.execute('Calling receiveHeaders after arrowhead600', () => Runners.hosts.federate.host);

	expect(result).to.be.equal(RECEIVE_HEADER_CALLED_TOO_SOON);
	expect(Number(result)).to.be.equal(RECEIVE_HEADER_CALLED_TOO_SOON);

01-04-54-post-iris-call-receive-headers / 01-04-55-post-iris-call-receive-header #26

01-04-54-post-iris-call-receive-headers / 01-04-55-post-iris-call-receive-header #26

Conversation

jpikora-iov commented Dec 19, 2023

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

jeremy-then left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpikora-iov commented Jan 3, 2024

jeremy-then commented Jan 3, 2024

jeremy-then 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

jeremy-then left a comment

Choose a reason for hiding this comment

jeremy-then left a comment

Choose a reason for hiding this comment

jeremy-then left a 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

sonarqubecloud bot commented Jan 5, 2024

Quality Gate passed

marcos-iov commented Jan 5, 2024

jpikora-iov commented Jan 8, 2024

jeremy-then left a comment

Choose a reason for hiding this comment