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

Create new Federation and the complete process until it is confirmed and activated #2925

Open
wants to merge 10 commits into
base: federation_change_federeration_integration
Choose a base branch
from
Open

Create new Federation and the complete process until it is confirmed and activated #2925

wants to merge 10 commits into from

Conversation

apancorb
Copy link
Contributor

@apancorb apancorb commented Jan 10, 2025 •
edited
Loading

Description

The idea is to provide building blocks that can be reused while creating new tests. This focuses on creating a new active and retiring federation.

@apancorb apancorb self-assigned this Jan 10, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 10, 2025 •
edited
Loading

Dependency Review

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

OpenSSF Scorecard

PackageVersionScoreDetails

Scanned Manifest Files

@apancorb apancorb changed the title Fix/powpeg migration tests common path Create new Federation and the complete process until it is confirmed and activated Jan 10, 2025
@apancorb apancorb marked this pull request as ready for review January 10, 2025 15:33
@apancorb apancorb requested a review from a team as a code owner January 10, 2025 15:33
jeremy-then
jeremy-then reviewed Jan 10, 2025
View reviewed changes
rskj-core/src/test/java/co/rsk/peg/federation/FederationChangeIT.java Outdated
case P2SH_ERP -> {
originalFederation = FederationFactory.buildP2shErpFederation(
originalFederationArgs, erpPubKeys, activationDelay);
// TODO: CHECK REDEEMSCRIPT
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this TODO be here still?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed, thanks

jeremy-then
jeremy-then approved these changes Jan 10, 2025
View reviewed changes
Copy link
Contributor

@jeremy-then jeremy-then left a comment

Choose a reason for hiding this comment

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

LGTM.

@apancorb apancorb force-pushed the fix/powpeg-migration-tests-common-path branch from 1ba5cc7 to 1191212 Compare January 14, 2025 10:48
julia-zack
julia-zack reviewed Jan 14, 2025
View reviewed changes
rskj-core/src/test/java/co/rsk/peg/federation/FederationChangeIT.java
addNewBtcBlockOnTipOfChain(btcBlockStore);
repository.save();

bridgeEventLogger = new BridgeEventLoggerImpl(BRIDGE_CONSTANTS, activations, new ArrayList<>());
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
bridgeEventLogger = new BridgeEventLoggerImpl(BRIDGE_CONSTANTS, activations, new ArrayList<>());
bridgeEventLogger = new BridgeEventLoggerImpl(BRIDGE_CONSTANTS, activations, logs);

we probably wanna define the logs at class level, to assert federation change related logged events. Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, if in a future PR we need will make it an instance variable.

Copy link
Contributor

@julia-zack julia-zack Jan 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

wouldn't be worth to check for the commit federation event for example? I mean, in this pr

@apancorb apancorb force-pushed the fix/powpeg-migration-tests-common-path branch from 250c82b to 0f5161b Compare January 15, 2025 11:11
julia-zack
julia-zack approved these changes Jan 15, 2025
View reviewed changes
Copy link
Contributor

@julia-zack julia-zack left a comment

Choose a reason for hiding this comment

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

nice work

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeremy-then jeremy-then jeremy-then approved these changes

@julia-zack julia-zack julia-zack approved these changes

Assignees

@apancorb apancorb

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@apancorb @julia-zack @jeremy-then