Skip to content

Conversation

@julianlen
Copy link
Contributor

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:

@julianlen julianlen requested a review from a team as a code owner December 23, 2025 16:47
@github-actions
Copy link

github-actions bot commented Dec 23, 2025

Dependency Review

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

Scanned Files

None

@julianlen julianlen requested review from Copilot and removed request for a team December 23, 2025 16:47
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors the getRetiringFederatorBtcPublicKey method to return an Optional<BtcECKey> instead of a nullable byte[]. This change improves type safety by making the absence of a retiring federator public key explicit through the Optional type, eliminating null checks and promoting better API design.

Key Changes:

  • Modified method signature to return Optional<BtcECKey> instead of byte[]
  • Updated implementations to return Optional.empty() instead of null when no retiring federation exists
  • Refactored empty byte array returns to use EMPTY_BYTE_ARRAY constant throughout the Bridge class

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
FederationSupport.java Updated interface method signature and documentation to return Optional
FederationSupportImpl.java Modified implementation to return Optional and handle empty case with Optional.empty()
BridgeSupport.java Updated method signature to match new return type
Bridge.java Refactored to use Optional API and replaced inline empty byte arrays with EMPTY_BYTE_ARRAY constant
FederationSupportImplTest.java Updated test assertions to check for empty Optional instead of null values
BridgeSupportTest.java Modified mock setup and assertions to work with Optional return type
BridgeSupportIT.java Updated integration tests to assert on Optional presence and extract values properly
BridgeIT.java Refactored test to use parameterized testing and updated mock to return Optional

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

Comment on lines 521 to 522
assertTrue(bridgeSupport.getRetiringFederatorBtcPublicKey(0).isPresent());
assertEquals(bridgeSupport.getRetiringFederatorBtcPublicKey(0).get(), publicKey);
Copy link

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

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

The method getRetiringFederatorBtcPublicKey(0) is called twice consecutively on lines 521 and 522. Store the result in a variable to avoid redundant method calls.

Suggested change
assertTrue(bridgeSupport.getRetiringFederatorBtcPublicKey(0).isPresent());
assertEquals(bridgeSupport.getRetiringFederatorBtcPublicKey(0).get(), publicKey);
Optional<BtcECKey> actualPublicKeyOptional = bridgeSupport.getRetiringFederatorBtcPublicKey(0);
assertTrue(actualPublicKeyOptional.isPresent());
assertEquals(actualPublicKeyOptional.get(), publicKey);

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@julianlen julianlen requested a review from Copilot December 23, 2025 21:46
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.


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

Comment on lines 891 to 892
Optional<BtcECKey> publicKey = bridgeSupport.getRetiringFederatorBtcPublicKey(index);
return publicKey.map(BtcECKey::getPubKey).orElse(EMPTY_BYTE_ARRAY);
Copy link

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

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

The variable name publicKey is ambiguous since it stores an Optional<BtcECKey> (the full key object), not the raw public key bytes. Consider renaming to retiringFederatorKey or retiringFederatorBtcKey to better reflect what it contains.

Suggested change
Optional<BtcECKey> publicKey = bridgeSupport.getRetiringFederatorBtcPublicKey(index);
return publicKey.map(BtcECKey::getPubKey).orElse(EMPTY_BYTE_ARRAY);
Optional<BtcECKey> retiringFederatorBtcKey = bridgeSupport.getRetiringFederatorBtcPublicKey(index);
return retiringFederatorBtcKey.map(BtcECKey::getPubKey).orElse(EMPTY_BYTE_ARRAY);

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@sonarqubecloud
Copy link

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants