Add fullBlockResponse param to Access API block requests
#135
Conversation
WalkthroughThe pull request modifies the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
SDK Examples Unit Test Results39 files 39 suites 6m 49s ⏱️ Results for commit abffae9. ♻️ This comment has been updated with latest results. |
Common Integration Test Results1 files 1 suites 7s ⏱️ Results for commit abffae9. ♻️ This comment has been updated with latest results. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
sdk/src/main/kotlin/org/onflow/flow/sdk/AsyncFlowAccessApi.kt (2)
19-19: Consider future interface consolidation.While the change is correct, the three block retrieval methods share identical parameter patterns. Consider introducing a common interface or base class in the future to reduce duplication and ensure consistent evolution of these related methods.
Example future refactor:
interface BlockRetrieval { fun getBlock(identifier: BlockIdentifier, fullBlockResponse: Boolean = false): CompletableFuture<FlowAccessApi.AccessApiCallResponse<FlowBlock?>> } sealed class BlockIdentifier { data class ByHeight(val height: Long): BlockIdentifier() data class ById(val id: FlowId): BlockIdentifier() object Latest: BlockIdentifier() }
15-19: Add KDoc documentation for the new parameter.Please add documentation explaining the purpose and behavior of the
fullBlockResponseparameter. This will help API consumers understand when to use this parameter and what differences to expect in the response.Example:
/** * @param fullBlockResponse When true, returns the complete block data. When false (default), * returns a minimal block response for better performance. */sdk/src/main/kotlin/org/onflow/flow/sdk/FlowAccessApi.kt (2)
31-31: LGTM! Consider documenting the performance implications.The parameter addition is consistent with other block retrieval methods. Consider adding documentation about potential performance differences between full and partial block responses to help API consumers make informed decisions.
Looking at the implementation details from the ripgrep results, I can now generate the final response.
Deprecation is correct but implementations differ slightly
The implementations in
FlowAccessApiImpl.ktshow that while both methods have the same signature and return type, they actually make different gRPC calls:
getAccountByAddressusesapi.getAccountwithAccess.GetAccountRequestgetAccountAtLatestBlockusesapi.getAccountAtLatestBlockwithAccess.GetAccountAtLatestBlockRequestThis suggests that the deprecation notice might need revision since the methods don't behave identically as claimed. The underlying gRPC calls are different, which could potentially have different behaviors or guarantees.
Locations that need attention:
sdk/src/main/kotlin/org/onflow/flow/sdk/FlowAccessApi.kt: The deprecation message stating "Behaves identically to getAccountAtLatestBlock" should be reviewedsdk/src/main/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImpl.kt: The implementations show different underlying gRPC calls🔗 Analysis chain
Line range hint
39-43: LGTM! Verify method equivalence.The deprecation notice is clear and provides a proper migration path. The ReplaceWith directive will help IDEs automatically update usage.
Let's verify the implementation equivalence:
Let me try a different approach to verify the implementation equivalence.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for both method implementations to confirm they behave identically ast-grep --pattern 'fun getAccountByAddress($$$) { $$$ }' ast-grep --pattern 'fun getAccountAtLatestBlock($$$) { $$$ }'Length of output: 241
Script:
#!/bin/bash # Search for both method implementations using ripgrep for interface and implementation rg "fun getAccountByAddress" -A 5 rg "fun getAccountAtLatestBlock" -A 5Length of output: 6811
sdk/src/main/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImpl.kt (2)
Line range hint
118-134: Consider extracting common block response handling logic.The implementation is correct, but there's duplicate error handling logic across all three block retrieval methods.
Consider extracting the common response handling logic into a private helper method:
private fun handleBlockResponse( response: Access.BlockResponse, errorMessage: String ): FlowAccessApi.AccessApiCallResponse<FlowBlock> = if (response.hasBlock()) { FlowAccessApi.AccessApiCallResponse.Success(FlowBlock.of(response.block)) } else { FlowAccessApi.AccessApiCallResponse.Error(errorMessage) }This would reduce duplication and make the code more maintainable.
Line range hint
192-192: Enhance deprecation notice with migration timeline.Consider adding more context to the deprecation notice, such as when this method will be removed and why
getAccountAtLatestBlockis preferred.Example enhancement:
-@Deprecated("Behaves identically to getAccountAtLatestBlock", replaceWith = ReplaceWith("getAccountAtLatestBlock")) +@Deprecated( + message = "Behaves identically to getAccountAtLatestBlock. Will be removed in version X.Y.Z.", + replaceWith = ReplaceWith("getAccountAtLatestBlock") +)sdk/src/main/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImpl.kt (1)
130-130: Consider adding KDoc documentation for the new parameter.The
fullBlockResponseparameter would benefit from documentation explaining its purpose and impact on the response.Apply this documentation to each method:
+ /** + * @param fullBlockResponse When true, returns the complete block data including collections, + * guarantees, and seals. Defaults to false for backward compatibility. + */Also applies to: 156-156, 186-186
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Files that changed from the base of the PR and between db9e3d1 and 1e9c9fc0eefe37a8914fcbcf6971bbb5d19bdbbd.
📒 Files selected for processing (4)
- sdk/src/main/kotlin/org/onflow/flow/sdk/AsyncFlowAccessApi.kt (1 hunks)
- sdk/src/main/kotlin/org/onflow/flow/sdk/FlowAccessApi.kt (1 hunks)
- sdk/src/main/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImpl.kt (3 hunks)
- sdk/src/main/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImpl.kt (2 hunks)
🔇 Additional comments (8)
sdk/src/main/kotlin/org/onflow/flow/sdk/AsyncFlowAccessApi.kt (2)
17-17: LGTM! Consistent with API pattern.The signature change follows the same pattern as getLatestBlock and maintains proper nullability.
15-15: LGTM! Verify integration with existing clients.The signature change maintains backward compatibility through default parameter value and follows Kotlin conventions.
Let's verify existing usage to ensure no breaking changes:
✅ Verification successful
Based on the search results, I can now generate the final response since I have sufficient information about the usage of
getLatestBlockacross the codebase.✅ Signature change is safe and properly implemented
The verification shows that all existing calls to
getLatestBlock()will continue to work without modification because:
- All current usages either specify only the
sealedparameter or use no parameters- The new
fullBlockResponseparameter has a default value offalse- The change has been consistently implemented across the interface (
AsyncFlowAccessApi), implementation (AsyncFlowAccessApiImpl), and test classes🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing getLatestBlock calls to verify compatibility rg "getLatestBlock\(" --type kotlinLength of output: 3035
sdk/src/main/kotlin/org/onflow/flow/sdk/FlowAccessApi.kt (2)
29-29: LGTM! Parameter signature is consistent.The
fullBlockResponseparameter addition maintains consistency with other block retrieval methods.
27-27: LGTM! Verify parameter usage in implementations.The addition of
fullBlockResponseparameter with a default value offalsemaintains backward compatibility while enhancing block retrieval functionality.Let's verify the parameter usage in implementations:
#!/bin/bash # Search for implementations of getLatestBlock to ensure consistent parameter usage ast-grep --pattern 'fun getLatestBlock($_, $_)'sdk/src/main/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImpl.kt (2)
Line range hint
100-116: LGTM! Implementation maintains robust error handling.The addition of
fullBlockResponseparameter is implemented correctly while preserving the existing null-check and error handling patterns.
86-98: LGTM! Verify integration with upstream callers.The implementation correctly incorporates the new
fullBlockResponseparameter and maintains consistent error handling.Let's verify the impact on existing callers:
sdk/src/main/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImpl.kt (2)
130-138: LGTM! The implementation is consistent across all block-related methods.The addition of the
fullBlockResponseparameter is implemented consistently acrossgetLatestBlock,getBlockById, andgetBlockByHeightmethods, maintaining the same pattern of error handling and response processing.Also applies to: 156-164, 186-194
130-138: Verify backward compatibility of existing clients.The new parameter has been added with proper default values, but let's verify that existing clients aren't affected.
✅ Verification successful
Based on the search results, I can now generate the final response as I have sufficient information about the usage of these methods across the codebase.
The new parameter is backward compatible with existing clients
The search results confirm that the new
fullBlockResponseparameter has been added with a default value offalseacross all block-related methods (getLatestBlock,getBlockById,getBlockByHeight). All existing client calls found in the codebase are compatible with this change because:
- The parameter is optional with a default value of
false- All existing calls either:
- Use only the required parameters (e.g.,
getBlockById(blockId))- Or specify only the first optional parameter (e.g.,
getLatestBlock(true))- No existing calls would be affected by the addition of the new parameter
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing calls to these methods to ensure they work with the new parameter echo "Searching for existing method calls..." rg -t kotlin -t java "getLatestBlock\s*\([^)]*\)" -A 1 rg -t kotlin -t java "getBlockById\s*\([^)]*\)" -A 1 rg -t kotlin -t java "getBlockByHeight\s*\([^)]*\)" -A 1Length of output: 12944
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
java-example/src/main/java/org/onflow/examples/java/getBlock/GetBlockAccessAPIConnector.java (1)
Line range hint
1-48: Consider enhancing example implementation to showcase API capabilities.Since this class is in the
examplespackage, it should demonstrate the full capabilities of the Flow Access API. Currently, the implementation hides the newfullBlockResponsefunctionality by hardcoding it tofalse. Consider:
- Exposing the parameter in all method signatures
- Adding example usage demonstrating both full and partial block responses
- Adding comments explaining when to use each option
This would make the example more valuable for developers learning to use the SDK.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Files that changed from the base of the PR and between 1e9c9fc0eefe37a8914fcbcf6971bbb5d19bdbbd and 5262ee47c990df54a1a0ee4aefed2fbb53555b08.
📒 Files selected for processing (1)
- java-example/src/main/java/org/onflow/examples/java/getBlock/GetBlockAccessAPIConnector.java (3 hunks)
java-example/src/main/java/org/onflow/examples/java/getBlock/GetBlockAccessAPIConnector.java
Show resolved
Hide resolved
java-example/src/main/java/org/onflow/examples/java/getBlock/GetBlockAccessAPIConnector.java
Show resolved
Hide resolved
java-example/src/main/java/org/onflow/examples/java/getBlock/GetBlockAccessAPIConnector.java
Show resolved
Hide resolved
lealobanov
force-pushed
the
full-block-response-protobuf
branch
from
7861fe9 to
aab430e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
java-example/src/test/java/org/onflow/examples/java/getCollection/GetCollectionAccessAPIConnectorTest.java (2)
37-37: Consider adding parameter documentation.The method call has been correctly updated to include the new
fullBlockResponseparameter. However, consider adding a comment to explain the meaning of both boolean parameters for better maintainability.+ // Get latest sealed block (sealed=true) without full block details (fullBlockResponse=false) FlowAccessApi.AccessApiCallResponse<FlowBlock> response = accessAPI.getLatestBlock(true, false);
Line range hint
37-45: Consider adding test coverage for fullBlockResponse=true.Since this test only verifies collection guarantees, it appropriately uses
fullBlockResponse=false. However, to ensure comprehensive test coverage of the new parameter, consider adding a separate test case that usesfullBlockResponse=trueand verifies the additional block details are present.Here's a suggested test method:
@Test public void canFetchLatestBlockWithFullResponse() { FlowAccessApi.AccessApiCallResponse<FlowBlock> response = accessAPI.getLatestBlock(true, true); assertTrue(response instanceof FlowAccessApi.AccessApiCallResponse.Success); FlowBlock block = ((FlowAccessApi.AccessApiCallResponse.Success<FlowBlock>) response).getData(); // Verify additional block details that should only be present with fullBlockResponse=true assertNotNull(block.getExecutionResult()); // Add more assertions based on what constitutes a "full" block response }sdk/src/main/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImpl.kt (1)
130-138: Consider adding documentation for the new parameter.While the implementation is correct, it would be helpful to add KDoc comments explaining the purpose and behavior of the
fullBlockResponseparameter.Add documentation like this to each method:
+ /** + * @param fullBlockResponse When true, returns the complete block data. Defaults to false for backward compatibility. + */ override fun getLatestBlock(sealed: Boolean, fullBlockResponse: Boolean): CompletableFuture<FlowAccessApi.AccessApiCallResponse<FlowBlock>>Also applies to: 207-215, 237-245
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Files that changed from the base of the PR and between 5262ee47c990df54a1a0ee4aefed2fbb53555b08 and aab430e.
📒 Files selected for processing (6)
- java-example/src/main/java/org/onflow/examples/java/getBlock/GetBlockAccessAPIConnector.java (3 hunks)
- java-example/src/test/java/org/onflow/examples/java/getCollection/GetCollectionAccessAPIConnectorTest.java (1 hunks)
- sdk/src/main/kotlin/org/onflow/flow/sdk/AsyncFlowAccessApi.kt (1 hunks)
- sdk/src/main/kotlin/org/onflow/flow/sdk/FlowAccessApi.kt (1 hunks)
- sdk/src/main/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImpl.kt (3 hunks)
- sdk/src/main/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImpl.kt (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- java-example/src/main/java/org/onflow/examples/java/getBlock/GetBlockAccessAPIConnector.java
- sdk/src/main/kotlin/org/onflow/flow/sdk/AsyncFlowAccessApi.kt
- sdk/src/main/kotlin/org/onflow/flow/sdk/FlowAccessApi.kt
- sdk/src/main/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImpl.kt
🔇 Additional comments (2)
sdk/src/main/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImpl.kt (2)
130-138: Implementation looks good and maintains consistency!The changes consistently implement the
fullBlockResponseparameter across all block-related methods, maintaining proper error handling and following the existing patterns.Also applies to: 207-215, 237-245
130-138: Verify the default value behavior.The
fullBlockResponseparameter's default value offalseensures backward compatibility, but let's verify this behavior in the codebase.✅ Verification successful
Based on the search results, I can now generate the final response as I have sufficient information to verify the default value behavior:
Default value behavior for
fullBlockResponseparameter is correctly implemented and used consistentlyThe codebase shows that:
- All block-related methods (
getLatestBlock,getBlockById,getBlockByHeight) consistently definefullBlockResponsewith a default value offalsein both sync and async APIs- All existing calls in test and example code work without explicitly passing the
fullBlockResponseparameter, confirming backward compatibility- The implementation in
AsyncFlowAccessApiImpl.ktandFlowAccessApiImpl.ktcorrectly handles this parameter by passing it to the underlying gRPC API🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing calls to these methods to ensure they work with the new parameter rg -A 2 "getLatestBlock|getBlockById|getBlockByHeight" --type kotlinLength of output: 29474
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
java-example/src/test/java/org/onflow/examples/java/getAccountBalance/GetAccountBalanceAccessAPIConnectorTest.java (1)
42-42: Add test coverage for fullBlockResponse=true scenario.While the parameter update is correct, consider adding a test case that verifies the behavior when
fullBlockResponseis set to true, ensuring the new functionality is properly tested.Would you like me to help generate a test case that covers the
fullBlockResponse=truescenario?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- java-example/src/test/java/org/onflow/examples/java/getAccountBalance/GetAccountBalanceAccessAPIConnectorTest.java (2 hunks)
🔇 Additional comments (2)
java-example/src/test/java/org/onflow/examples/java/getAccountBalance/GetAccountBalanceAccessAPIConnectorTest.java (2)
61-61: LGTM! Parameter update is consistent.The modification correctly implements the new API signature while maintaining the test's original purpose of comparing balances between the latest block and a specific height.
Line range hint
42-61: Verify consistent parameter usage across the codebase.Let's ensure all
getLatestBlockcalls have been updated consistently with the new parameter.✅ Verification successful
Based on the search results, I can now generate the final response:
All
getLatestBlockcalls are consistently using the updated parametersThe verification shows that:
- Java implementations consistently use both parameters:
getLatestBlock(true, false)- Kotlin implementations use a single parameter:
getLatestBlock(true)or parameter-lessgetLatestBlock()- This is expected as Kotlin has default parameters while Java requires explicit parameters
- The parameter ordering and values are consistent across similar use cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining calls to getLatestBlock that might need updating # and verify parameter consistency across the codebase # Search for all getLatestBlock method calls echo "Checking getLatestBlock method calls:" rg "getLatestBlock\(" --type java # Search for potential test files that might need similar updates echo -e "\nChecking for other test files that might need updates:" fd -e java -e kt --full-path ".*Test\.java$|.*Test\.kt$" | xargs rg "getLatestBlock"Length of output: 6190
Closes: #109
Description
fullBlockResponseboolean field to get block Access API methodsFor contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
Deprecations
getAccountByAddressmethod is now deprecated; users are encouraged to usegetAccountAtLatestBlockinstead for consistency.