Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Changes the federation from the start. #78

Merged
merged 6 commits into from
Sep 17, 2024
Merged

Changes the federation from the start. #78

merged 6 commits into from
Sep 17, 2024

Conversation

jeremy-then
Copy link
Contributor

Change federation from start

@jeremy-then jeremy-then self-assigned this Sep 11, 2024
@jeremy-then jeremy-then requested a review from a team as a code owner September 11, 2024 15:43
marcos-iov
marcos-iov reviewed Sep 11, 2024
View reviewed changes
Copy link
Collaborator

@marcos-iov marcos-iov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, well done! Super nice and clean

tests/00_00_04-change-federation.js Outdated
Comment on lines 23 to 25
const expectedNewFederationAddress = '2NGJ9Rhk5KqK1RwMZm7Uph6nhGFL5MYtpJo';

const expectedNewFederationErpRedeemScript = '0x64532102cd53fc53a07f211641a677d250f6de99caf620e8e77071e811a28b3bcddf0be12102f80abfd3dac069887f974ac033cb62991a0ed55b9880faf8b8cbd713b75d649e2103488898918c76758e52842c700060adbbbd0a38aa836838fd7215147b924ef7dc210362634ab57dae9cb373a5d536e66a8c4f67468bbcfb063809bab643072d78a1242103c5946b3fbae03a654237da863c9ed534e0878657175b132b8ca630f245df04db55ae6702f401b2755321029cecea902067992d52c38b28bf0bb2345bda9b21eca76b16a17c477a64e433012103284178e5fbcc63c54c3b38e3ef88adf2da6c526313650041b0ef955763634ebd2103776b1fd8f86da3c1db3d69699e8250a15877d286734ea9a6da8e9d8ad25d16c12103ab0e2cd7ed158687fc13b88019990860cdb72b1f5777b58513312550ea1584bc2103b9fc46657cf72a1afa007ecf431de1cd27ff5cc8829fa625b66ca47b967e6b2455ae68';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we get these values using powpeg-redeem-script-parser? So it's not hardcoded here, since this basically depends on the keys used in the config file. A change in the config would make this test fail

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

tests/00_00_04-change-federation.js Outdated

it('should activate federation', async () => {

const federationActivationBlockNumber = commitFederationEvent.arguments.activationHeight;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't get this value from the event. I think we should have a constant with the fed activation age, and assert that the event is emitted with the expected value

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd still change the way this is calculated, to be clear.

federationActivationBlockNumber = blockNumber + fedActivationAge;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

marcos-iov
marcos-iov reviewed Sep 12, 2024
View reviewed changes
tests/00_00_04-change-federation.js Outdated

it('should complete retiring the old federation', async () => {

const blocksToMineToRetireFederation = FUNDS_MIGRATION_AGE_SINCE_ACTIVATION_BEGIN + FUNDS_MIGRATION_AGE_SINCE_ACTIVATION_END;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this right? Shouldn't it just be FUNDS_MIGRATION_AGE_SINCE_ACTIVATION_END?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

tests/00_00_04-change-federation.js Outdated
const { getBridge, getLatestActiveForkName } = require('../lib/precompiled-abi-forks-util');
const { btcToWeis } = require('@rsksmart/btc-eth-unit-converter');
const { removePrefix0x, ensure0x, splitStringIntoChunks } = require('../lib/utils');
const bridgeTxParser = require('bridge-transaction-parser-fingerroot500');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use the fingerroot one? We should be aiming to get rid of this dependency actually since anything pre arrowhead won't be tested here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

tests/00_00_04-change-federation.js Outdated
Comment on lines 26 to 28
const expectedPendingFederationHash = '0x6f45ac8763317802a9a9c540b58af03e577bb4af14209f8a690d501e21c68ace';

const expectedNewFederationAddress = '2NGJ9Rhk5KqK1RwMZm7Uph6nhGFL5MYtpJo';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we calculate these values using the redeem script parser library instead of hardcoding them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

tests/00_00_04-change-federation.js
const newActiveFederationAddress = commitFederationEvent.arguments.newFederationBtcAddress;
expect(newActiveFederationAddress).to.be.equal(expectedNewFederationAddress, 'The new federation address in the commit_federation event should be the expected one.');

// Assert the new federation btc public keys in the commit federation event are the expected ones.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also assert the old federation public keys in the event are the expected ones? Looks like it's the only thing missing to validate in the event

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

tests/00_00_04-change-federation.js Outdated

it('should activate federation', async () => {

const federationActivationBlockNumber = commitFederationEvent.arguments.activationHeight;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd still change the way this is calculated, to be clear.

federationActivationBlockNumber = blockNumber + fedActivationAge;

tests/00_00_04-change-federation.js Outdated
expect(newFederationAddress).to.be.equal(expectedNewFederationAddress, 'The new active federation address should be the expected one.');

// Assert the active federation redeem script is the expected ones.
const newActiveFederaetionErpRedeemScript = removePrefix0x(await bridge.methods.getActivePowpegRedeemScript().call());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small typo here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

marcos-iov
marcos-iov reviewed Sep 16, 2024
View reviewed changes
lib/rsk-utils.js Outdated
@@ -2,7 +2,7 @@ const expect = require('chai').expect;
const { getBridgeState } = require('@rsksmart/bridge-state-data-parser');
const { getBridge, getLatestActiveForkName } = require('./precompiled-abi-forks-util');
const hopBridgeTxParser = require('bridge-transaction-parser-hop400');
const fingerrootBridgeTxParser = require('bridge-transaction-parser-fingerroot500');
const fingerrootBridgeTxParser = require('bridge-transaction-parser');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

package.json Outdated
@@ -19,7 +19,7 @@
"@rsksmart/powpeg-redeemscript-parser": "^1.0.0",
"bn.js": "^4.11.8",
"bridge-transaction-parser-hop400": "github:rsksmart/bridge-transaction-parser#v0.4.0-beta",
"bridge-transaction-parser-fingerroot500": "github:rsksmart/bridge-transaction-parser#v0.5.0-beta",
"bridge-transaction-parser": "github:rsksmart/bridge-transaction-parser#v0.5.0-beta",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why version 0.5? Shouldn't it be the latest one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

package-lock.json Outdated
@@ -1,155 +1,89 @@
{
"name": "rootstock-integration-tests",
"version": "1.0.0",
"lockfileVersion": 3,
"lockfileVersion": 1,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Careful

Be sure to review the code you submit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

@jeremy-then jeremy-then force-pushed the change-federation-from-start branch from fcc5d43 to c2ca414 Compare September 17, 2024 00:55
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
2 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 SonarCloud

@marcos-iov marcos-iov merged commit da1f3f5 into rits-refactors-9-2024-integration Sep 17, 2024
5 checks passed
@marcos-iov marcos-iov deleted the change-federation-from-start branch September 17, 2024 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nathanieliov nathanieliov nathanieliov approved these changes

@marcos-iov marcos-iov marcos-iov approved these changes

Assignees

@jeremy-then jeremy-then

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jeremy-then @marcos-iov @nathanieliov