turn getRetiringFederationAddress to return an optional, added two te… #3428
Conversation
…sts to BridgeIT to check the returned string is correct
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
Pull request overview
This PR refactors the getRetiringFederationAddress() method to return an Optional<Address> instead of a nullable Address, improving null safety and making the API more explicit about the absence of a retiring federation address.
- Changed the return type from
AddresstoOptional<Address>across the federation support interfaces and implementations - Updated all test cases to handle the new Optional return type
- Added two new integration tests in BridgeIT to verify correct behavior for both empty and present retiring federation addresses
Reviewed changes
Copilot reviewed 10 out of 10 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<Address> |
| FederationSupportImpl.java | Changed implementation to return Optional<Address> instead of null |
| BridgeSupport.java | Updated method signature and added comment about funds migration dependency |
| Bridge.java | Refactored to use Optional pattern with map().orElse() instead of null check |
| BridgeIT.java | Added two new tests verifying empty string for absent federation and correct address for present federation |
| FederationChangeIT.java | Updated assertions to check Optional presence before accessing value |
| BridgeSupportIT.java | Updated test to verify Optional wrapper is returned |
| BridgeSupportTest.java | Updated mock setup to return Optional wrapper |
| BridgeTest.java | Updated mock setup to return Optional wrapper |
| FederationSupportImplTest.java | Updated all test cases to use Optional assertions instead of null checks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Optional<Address> address = bridgeSupport.getRetiringFederationAddress(); | ||
|
|
||
| if (address == null) { | ||
| // When there's no address, empty string is returned | ||
| return ""; | ||
| } | ||
| // When there's no address, empty string is returned | ||
| return address.map(Address::toBase58).orElse(""); |
There was a problem hiding this comment.
The blank lines at 864 and 867 should be removed to improve code readability. The comment should be placed directly above the return statement it describes.
| "[processFundsMigration] Unable to complete retiring federation migration. Balance left: {} in {}", | ||
| retiringFederationWallet.getBalance().toFriendlyString(), | ||
| getRetiringFederationAddress() | ||
| getRetiringFederationAddress().get() |
There was a problem hiding this comment.
Calling .get() on the Optional without checking if it's present could throw NoSuchElementException. The comment above states 'Funds cannot be migrated if there is no retiring federation', but the code doesn't guard against an empty Optional. Consider using .orElseThrow() with a descriptive exception or add a presence check before this line.
There was a problem hiding this comment.
According to the comment above, there might not be a retiring federation at this point. If that's the case then we want to handle this case and avoid an exception here.
If it is not the case perhaps we could get the address from the retiring federation wallet? I believe there is a watched address value
There was a problem hiding this comment.
The comment above is wrong. It says "Funds cannot be migrated if there is no retiring federation" but in reality, that part of the code is catching any exception from migrateFunds after the migration age. At that point, a retiring federation exists. Otherwise, the method would return in
Using the watched address could do, I guess. It returns a list of watched addresses. At that point, it is just the retiring federation. wdyt?
There was a problem hiding this comment.
Getting the watched address from the wallet is probably the safest option yes
There was a problem hiding this comment.
Agree, let's get it from the watchedAddresses method
|
Regarding the comment #3428 (comment), no migration should happen if there is no retiring federation. This is exactly why in I left the ".get()" without checking its presence.I don't think it's worth it to have a line like
|
| availableUTXOs | ||
| ); | ||
| } catch (Exception e) { | ||
| // Funds cannot be migrated if there is no retiring federation |
There was a problem hiding this comment.
This comment is a bit confusing, do we expect an exception here if there is no retiring federation
| "[processFundsMigration] Unable to complete retiring federation migration. Balance left: {} in {}", | ||
| retiringFederationWallet.getBalance().toFriendlyString(), | ||
| getRetiringFederationAddress() | ||
| getRetiringFederationAddress().get() |
There was a problem hiding this comment.
According to the comment above, there might not be a retiring federation at this point. If that's the case then we want to handle this case and avoid an exception here.
If it is not the case perhaps we could get the address from the retiring federation wallet? I believe there is a watched address value
… shows the watchedAddresses
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
…sts to BridgeIT to check the returned string is correct
Description
Motivation and Context
How Has This Been Tested?
Types of changes
Checklist: