Turn retiring federation creation block number optional #3427
Conversation
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 getRetiringFederationCreationBlockNumber method to return an Optional<Long> instead of a primitive long, making the absence of a retiring federation more explicit and type-safe. Previously, the method returned -1 (via FEDERATION_NON_EXISTENT response code) when no retiring federation existed.
Key Changes:
- Changed return type from
longtoOptional<Long>in the interface and implementation - Updated all test cases to work with the new Optional-based API
- Preserved backward compatibility at the Bridge API level by unwrapping the Optional with the same sentinel value
Reviewed changes
Copilot reviewed 7 out of 7 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<Long> |
| FederationSupportImpl.java | Simplified implementation to return Optional directly instead of unwrapping with sentinel value |
| BridgeSupport.java | Updated method signature to match new interface |
| Bridge.java | Added unwrapping logic to maintain backward compatibility with existing API contract |
| FederationSupportImplTest.java | Updated test assertions to verify Optional.isEmpty() instead of checking sentinel value |
| BridgeSupportTest.java | Updated mock setup and assertions to use Optional |
| BridgeIT.java | Updated integration test to work with Optional return type |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| long retiringFederationCreationBlockNumber = federationSupport.getRetiringFederationCreationBlockNumber(); | ||
| Optional<Long> retiringFederationCreationBlockNumber = federationSupport.getRetiringFederationCreationBlockNumber(); | ||
| assertThat(retiringFederationCreationBlockNumber, is(oldFederation.getCreationBlockNumber())); |
There was a problem hiding this comment.
The assertion compares an Optional<Long> with a primitive long. This should be assertThat(retiringFederationCreationBlockNumber, is(Optional.of(oldFederation.getCreationBlockNumber()))) or use retiringFederationCreationBlockNumber.get() in the assertion.
| assertThat(retiringFederationCreationBlockNumber, is(oldFederation.getCreationBlockNumber())); | |
| assertThat(retiringFederationCreationBlockNumber, is(Optional.of(oldFederation.getCreationBlockNumber()))); |
| // Return the creation time in seconds from the epoch | ||
| return retiringFederationCreationTime.getEpochSecond(); | ||
| } | ||
|
|
There was a problem hiding this comment.
The method documentation should be updated to clarify that it returns the sentinel value FEDERATION_NON_EXISTENT.getCode() when no retiring federation exists, since this behavior is no longer obvious from the implementation.
| /** | |
| * Returns the block number at which the current retiring federation was created. | |
| * <p> | |
| * If there is no retiring federation, this method returns the sentinel value | |
| * {@link FederationChangeResponseCode#FEDERATION_NON_EXISTENT#getCode()}. | |
| * | |
| * @param args unused arguments array required by the precompiled contract interface | |
| * @return the retiring federation creation block number, or | |
| * {@code FederationChangeResponseCode.FEDERATION_NON_EXISTENT.getCode()} if | |
| * no retiring federation exists | |
| */ |
There was a problem hiding this comment.
I think it is suggesting this because other functions returning 'orElse' have some documentation.
What do you think if we just add some simple doc?
getRetiringFederationCreationTime, getRetiringFederationCreationBlockNumber.
There are others like getLockWhitelistAddress and getActivePowpegRedeemScript but are out of the scope of this ticket.
What do you think?
julianlen
force-pushed
the
getRetiringFederationCreationBlockNumber-optional
branch
from
eef0a10 to
a662892
Compare
|
marcos-iov
deleted the
getRetiringFederationCreationBlockNumber-optional
branch
Description
Motivation and Context
How Has This Been Tested?
Types of changes
Checklist: