FlowBlockSeal protobuf updates #122
Conversation
WalkthroughThe changes involve modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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 Results37 files 37 suites 6m 18s ⏱️ Results for commit 1f4ab77. ♻️ 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: 1
🧹 Outside diff range and nitpick comments (7)
sdk/src/test/kotlin/org/onflow/flow/sdk/models/FlowAggregatedSignatureTest.kt (3)
44-66: LGTM: Comprehensive test foroffunction with a minor suggestionThis test effectively verifies the
FlowAggregatedSignature.offunction by mocking the protobuf object and checking the conversion result. It covers both the size and content of the resulting lists.Consider adding a test case with multiple signatures and signer IDs to ensure the function handles lists correctly:
Mockito.`when`(aggregatedSignatureProto.verifierSignaturesList).thenReturn( listOf( com.google.protobuf.ByteString.copyFrom("signature1".toByteArray()), com.google.protobuf.ByteString.copyFrom("signature2".toByteArray()) ) ) Mockito.`when`(aggregatedSignatureProto.signerIdsList).thenReturn( listOf( com.google.protobuf.ByteString.copyFrom("signerId1".toByteArray()), com.google.protobuf.ByteString.copyFrom("signerId2".toByteArray()) ) ) val flowAggregatedSignature = FlowAggregatedSignature.of(aggregatedSignatureProto) assertEquals(2, flowAggregatedSignature.verifierSignatures.size) assertEquals(2, flowAggregatedSignature.signerIds.size) // Add assertions for individual elements
68-82: LGTM: Good test for builder function with suggestions for improvementThis test effectively verifies the
builder()function of theFlowAggregatedSignatureclass. It checks that the builder produces the correctByteStringrepresentations of the original data.Consider enhancing the test with the following improvements:
- Add a more complex case with multiple signatures and signer IDs:
val signature1 = FlowSignature("signature1".toByteArray()) val signature2 = FlowSignature("signature2".toByteArray()) val signerId1 = FlowId.of("signerId1".toByteArray()) val signerId2 = FlowId.of("signerId2".toByteArray()) val aggregatedSignature = FlowAggregatedSignature( verifierSignatures = listOf(signature1, signature2), signerIds = listOf(signerId1, signerId2) ) val builderResult = aggregatedSignature.builder() assertEquals(listOf(signature1.byteStringValue, signature2.byteStringValue), builderResult.verifierSignaturesList) assertEquals(listOf(signerId1.byteStringValue, signerId2.byteStringValue), builderResult.signerIdsList)
- Add a test for deep equality by converting the builder result back to a
FlowAggregatedSignature:val reconstructedSignature = FlowAggregatedSignature.of(builderResult.build()) assertEquals(aggregatedSignature, reconstructedSignature)These additions will provide more comprehensive coverage of the
builder()function and ensure that the round-trip conversion works correctly.
1-11: Consider adding more test cases and class documentationThe test class provides good coverage of the main functionalities of
FlowAggregatedSignature. To further improve the test suite, consider the following suggestions:
- Add a brief documentation comment for the class explaining its purpose:
/** * Unit tests for the FlowAggregatedSignature class. * These tests cover equality, hash code, conversion from protobuf, and the builder pattern. */ class FlowAggregatedSignatureTest { // ... existing code ... }
Add test cases for potential edge cases or error scenarios, such as:
- Empty lists of signatures or signer IDs
- Very large lists of signatures or signer IDs
- Invalid input data (e.g., mismatched sizes of signatures and signer IDs)
Consider adding a test for the
toString()method ofFlowAggregatedSignatureif it's implemented.These additions will enhance the robustness and documentation of your test suite.
sdk/src/test/kotlin/org/onflow/flow/sdk/models/FlowBlockSealTest.kt (4)
13-53: LGTM: Comprehensive equality and hash code test.This test thoroughly checks the
equalsandhashCodemethods ofFlowBlockSeal, including all properties. It's particularly valuable as it covers the newly added properties such asfinalState,resultId, andaggregatedApprovalSigs.Consider adding a negative test case where one property differs between the two instances to ensure the
equalsmethod correctly identifies inequality.
55-85: LGTM: Thorough test for protobuf conversion.This test effectively verifies the
FlowBlockSeal.offunction, ensuring correct conversion from the protobuf representation. It covers all properties, including the newly added ones likeresultIdandfinalState.Consider adding assertions for
executionReceiptSignatures,resultApprovalSignatures, andaggregatedApprovalSigsto ensure complete coverage of all properties during conversion.
87-120: LGTM: Comprehensive test for the builder function.This test effectively verifies the
FlowBlockSealbuilder functionality, ensuring all properties are correctly reconstructed, including the newly added ones likeresultIdandfinalState.Consider adding an assertion for
aggregatedApprovalSigsto ensure complete coverage of all properties in the builder test.
1-120: Great job on comprehensive test coverage for FlowBlockSeal!This new test file aligns well with the PR objectives, particularly in addressing the addition of missing protobuf fields to the
FlowBlockSealmodel. The tests cover equality, protobuf conversion, and builder functionality, including checks for newly added properties likefinalState,resultId, andaggregatedApprovalSigs.The test structure is clear, and the use of JUnit and Mockito is appropriate. The tests effectively verify the changes made to the
FlowBlockSealclass, ensuring its correctness and compatibility with the updated protobuf schema.To further enhance the test suite:
- Consider adding negative test cases for equality checks.
- Ensure all properties are asserted in the protobuf conversion and builder tests.
- If not already present elsewhere, consider adding integration tests that use actual protobuf serialization/deserialization to complement these unit tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- sdk/src/main/kotlin/org/onflow/flow/sdk/models.kt (1 hunks)
- sdk/src/test/kotlin/org/onflow/flow/sdk/models/FlowAggregatedSignatureTest.kt (1 hunks)
- sdk/src/test/kotlin/org/onflow/flow/sdk/models/FlowBlockSealTest.kt (1 hunks)
🧰 Additional context used
🔇 Additional comments (6)
sdk/src/test/kotlin/org/onflow/flow/sdk/models/FlowAggregatedSignatureTest.kt (1)
13-42: LGTM: Well-structured equality and hash code testThis test method effectively covers both positive and negative cases for equality and hash code consistency of the
FlowAggregatedSignatureclass. It uses clear variable names and follows Kotlin naming conventions.sdk/src/test/kotlin/org/onflow/flow/sdk/models/FlowBlockSealTest.kt (1)
1-11: LGTM: Import statements are appropriate and concise.The import statements cover all necessary classes for the tests, including JUnit assertions, Mockito for mocking, and relevant Flow SDK classes. No unused imports are present.
sdk/src/main/kotlin/org/onflow/flow/sdk/models.kt (4)
1204-1210: Ensure all new properties inFlowBlockSealare correctly initialized and mappedThe
FlowBlockSealdata class has been updated with new properties:
resultApprovalSignaturesfinalStateresultIdaggregatedApprovalSigsPlease verify that these properties are correctly initialized in all constructors and methods, and that they're accurately mapped from the protobuf objects and back.
1215-1221: Confirm accurate mapping in theof()method ofFlowBlockSealIn the
of()method, the new properties are being mapped from the protobufBlockSealobject:
resultApprovalSignaturesfromvalue.resultApprovalSignaturesListfinalStatefromvalue.finalStateresultIdfromvalue.resultIdaggregatedApprovalSigsfromvalue.aggregatedApprovalSigsListEnsure that these mappings correspond correctly to the intended fields in the protobuf schema.
1227-1233: Verify thebuilder()method correctly sets all new propertiesIn the
builder()method ofFlowBlockSeal, the new properties are being set:
.addAllResultApprovalSignatures(resultApprovalSignatures.map { it.byteStringValue }).setFinalState(UnsafeByteOperations.unsafeWrap(finalState)).setResultId(resultId.byteStringValue).addAllAggregatedApprovalSigs(aggregatedApprovalSigs.map { it.builder().build() })Please confirm that these properties are correctly assigned to the corresponding protobuf fields to ensure consistency during serialization.
1235-1259: Include new properties inequals()andhashCode()methodsThe
equals()andhashCode()methods inFlowBlockSealhave been updated to include the new properties:
resultApprovalSignaturesfinalStateresultIdaggregatedApprovalSigsThis is essential for ensuring object equality and correct behavior in collections. Verify that these methods accurately compare and hash the new properties.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
sdk/src/test/kotlin/org/onflow/flow/sdk/models/FlowBlockSealTest.kt (3)
55-105: LGTM: Thorough test for protobuf conversion with room for improvement.This test effectively verifies the
FlowBlockSeal.offunction, ensuring accurate conversion from a protobuf representation to aFlowBlockSealinstance. It covers all properties, including the newly added ones.Consider enhancing the test by adding more assertions for list properties:
- Check the size of
executionReceiptSignatures,resultApprovalSignatures, andaggregatedApprovalSigslists.- Verify the content of these lists more thoroughly, especially for
aggregatedApprovalSigs.Example:
assertEquals(1, flowBlockSeal.executionReceiptSignatures.size) assertEquals(FlowSignature("signature1".toByteArray()), flowBlockSeal.executionReceiptSignatures[0]) // Similar assertions for other list properties
107-140: LGTM: Comprehensive test for builder function with a minor improvement suggestion.This test effectively verifies the
FlowBlockSealbuilder function, ensuring all properties are correctly reconstructed. It covers all properties, including the newly added ones.Consider adding an assertion for the
aggregatedApprovalSigsproperty in the builder result. This would ensure complete coverage of all properties:assertEquals(1, builderResult.aggregatedApprovalSigsList.size) val builtAggregatedSig = builderResult.aggregatedApprovalSigsList[0] assertEquals(aggregatedSignature1.verifierSignatures.map { it.byteStringValue }, builtAggregatedSig.verifierSignaturesList) assertEquals(aggregatedSignature1.signerIds.map { it.byteStringValue }, builtAggregatedSig.signerIdsList)
1-140: Excellent test coverage for FlowBlockSeal with minor enhancement opportunities.The
FlowBlockSealTestclass provides comprehensive test coverage for theFlowBlockSealclass, including the newly added properties and functionalities. The tests are well-structured, using appropriate JUnit 5 and Mockito features.Key strengths:
- Thorough testing of equality and hash code functionality.
- Comprehensive verification of protobuf conversion.
- Detailed testing of the builder function.
These tests align well with the PR objectives, ensuring that the
FlowBlockSealmodel accurately reflects the expected structure defined in the protobuf schema.To further enhance the test suite:
- Consider adding edge cases, such as empty lists or null values where applicable.
- Implement parameterized tests for different scenarios to increase coverage.
- Add tests for any public methods of
FlowBlockSealthat are not currently covered.These enhancements will contribute to a more robust test suite, further ensuring the reliability of the
FlowBlockSealclass.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- sdk/src/test/kotlin/org/onflow/flow/sdk/models/FlowAggregatedSignatureTest.kt (1 hunks)
- sdk/src/test/kotlin/org/onflow/flow/sdk/models/FlowBlockSealTest.kt (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- sdk/src/test/kotlin/org/onflow/flow/sdk/models/FlowAggregatedSignatureTest.kt
🧰 Additional context used
🔇 Additional comments (1)
sdk/src/test/kotlin/org/onflow/flow/sdk/models/FlowBlockSealTest.kt (1)
14-53: LGTM: Comprehensive test for equality and hash code.This test thoroughly checks the equality and hash code functionality of
FlowBlockSeal. It covers all properties, including the newly added ones likefinalState,resultId, andaggregatedApprovalSigs. The test structure is clear and includes necessary assertions.
Common Integration Test Results1 files 1 suites 7s ⏱️ Results for commit b7308f1. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
sdk/src/main/kotlin/org/onflow/flow/sdk/models.kt (1)
1204-1260: LGTM! Consider adding documentation for new properties.The changes to
FlowBlockSealclass look good. The new propertiesfinalState,resultId, andaggregatedApprovalSigshave been added as per the PR objectives. The renaming ofidtoblockIdimproves clarity. Theequals()andhashCode()methods have been correctly updated to include the new properties.Consider adding KDoc comments for the new properties to improve code documentation and maintainability.
Get account balance endpoints
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (20)
kotlin-example/src/main/kotlin/org/onflow/examples/kotlin/getAccountBalance/GetAccountBalanceAccessAPIConnector.kt (1)
3-3: Consider using specific imports instead of wildcardWhile wildcard imports can be convenient, explicit imports make dependencies clearer and prevent potential naming conflicts.
-import org.onflow.flow.sdk.* +import org.onflow.flow.sdk.FlowAccessApi +import org.onflow.flow.sdk.FlowAddressjava-example/src/main/java/org/onflow/examples/java/getAccountBalance/GetAccountBalanceAccessAPIConnector.java (1)
6-11: Add class-level documentation and constructor validation.Consider adding:
- JavaDoc documentation explaining the class's purpose and usage
- Null validation in the constructor
Apply these changes:
+/** + * Connector class for retrieving account balances from the Flow blockchain using the Flow Access API. + * This class provides methods to fetch balances at the latest block or at a specific block height. + */ public class GetAccountBalanceAccessAPIConnector { private final FlowAccessApi accessAPI; public GetAccountBalanceAccessAPIConnector(FlowAccessApi accessAPI) { + if (accessAPI == null) { + throw new IllegalArgumentException("accessAPI cannot be null"); + } this.accessAPI = accessAPI; }kotlin-example/src/test/kotlin/org/onflow/examples/kotlin/getAccountBalance/GetAccountBalanceAccessAPIConnectorTest.kt (1)
39-39: Extract sealed block flag to a constant.The boolean parameter
trueingetLatestBlockcalls should be extracted to a named constant for better readability and maintainability.+ companion object { + private const val SEALED_BLOCK_ONLY = true + } + @Test fun `Can fetch account balance at a specific block height`() { val address = serviceAccount.flowAddress - val latestBlock = accessAPI.getLatestBlock(true) + val latestBlock = accessAPI.getLatestBlock(SEALED_BLOCK_ONLY)Also applies to: 60-60
java-example/src/test/java/org/onflow/examples/java/getAccountBalance/GetAccountBalanceAccessAPIConnectorTest.java (2)
30-36: Enhance assertion message for better test failure diagnosis.The assertion message could be more descriptive by including the actual balance value.
- Assertions.assertTrue(balance >= 0, "Balance at the latest block should be non-negative"); + Assertions.assertTrue(balance >= 0, + String.format("Balance at the latest block should be non-negative, but got: %d", balance));
30-76: Add test coverage for edge cases.The current test suite could benefit from additional test cases:
- Test with zero balance accounts
- Test with invalid block heights
- Test with maximum block height
- Test with non-existent accounts
Would you like me to help generate these additional test cases?
sdk/src/main/kotlin/org/onflow/flow/sdk/FlowAccessApi.kt (1)
29-32: Add KDoc documentation for the new balance retrieval methods.The method signatures look good and follow the interface's patterns. However, consider adding KDoc documentation to clarify:
- The denomination of the returned balance (e.g., FLOW tokens in smallest unit)
- Any constraints on the height parameter
- Error conditions that might trigger an Error response
Example documentation:
/** * Retrieves the account balance at the latest sealed block. * * @param address The Flow address to query * @return Success with balance in FLOW smallest denomination, or Error if the account doesn't exist */ fun getAccountBalanceAtLatestBlock(address: FlowAddress): AccessApiCallResponse<Long> /** * Retrieves the account balance at a specific block height. * * @param address The Flow address to query * @param height The block height to query (must be <= latest block height) * @return Success with balance in FLOW smallest denomination, or Error if the account/block doesn't exist */ fun getAccountBalanceAtBlockHeight(address: FlowAddress, height: Long): AccessApiCallResponse<Long>sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt (2)
144-171: Extract magic number to a named constant.The normalization factor
100_000_000Lappears to be a significant value for Flow token denomination. Consider extracting it to a named constant for better maintainability and documentation.+ private companion object { + // Number of decimal places in Flow token (1 FLOW = 10^8 decimal places) + const val FLOW_TOKEN_DECIMALS = 100_000_000L + } @Test fun `Can get account balance at latest block`() { // ... - val normalizedBalance = balanceResponse / 100_000_000L + val normalizedBalance = balanceResponse / FLOW_TOKEN_DECIMALS
144-211: Tests are well-structured but could benefit from test data variation.The tests effectively verify the new account balance retrieval functionality and maintain consistency with the existing test patterns. However, consider enhancing the test suite with:
- A parameterized test that verifies balance retrieval with different account states and balance values.
- Edge cases such as accounts with zero balance or maximum possible balance.
This would provide more comprehensive coverage while maintaining the existing clean architecture.
sdk/src/test/kotlin/org/onflow/flow/sdk/FlowAccessApiTest.kt (2)
91-119: Consider enhancing test coverage for getAccountBalanceAtLatestBlock.While the basic success and failure cases are well-implemented, consider adding the following test cases for better coverage:
- Edge cases: zero balance, maximum balance
- Invalid inputs: malformed addresses
- Consistent error message format validation
Example test case for zero balance:
@Test fun `Test getAccountBalanceAtLatestBlock with zero balance`() { val flowAccessApi = mock(FlowAccessApi::class.java) val flowAddress = FlowAddress("0x1234567890abcdef") // More realistic address val expectedBalance = 0L val response = FlowAccessApi.AccessApiCallResponse.Success(expectedBalance) `when`(flowAccessApi.getAccountBalanceAtLatestBlock(flowAddress)).thenReturn(response) val result = flowAccessApi.getAccountBalanceAtLatestBlock(flowAddress) assertEquals(response, result) verify(flowAccessApi).getAccountBalanceAtLatestBlock(flowAddress) }
121-151: Consider enhancing test coverage for getAccountBalanceAtBlockHeight.While the basic success and failure cases are well-implemented, consider adding the following test cases for better coverage:
- Edge cases: zero balance, maximum balance
- Invalid inputs: negative block height, non-existent block height
- Consistent error message format validation
Example test case for non-existent block height:
@Test fun `Test getAccountBalanceAtBlockHeight with non-existent block height`() { val flowAccessApi = mock(FlowAccessApi::class.java) val flowAddress = FlowAddress("0x1234567890abcdef") // More realistic address val nonExistentHeight = Long.MAX_VALUE val exception = RuntimeException("Block height not found") val response = FlowAccessApi.AccessApiCallResponse.Error( "Failed to get account balance: block height not found", exception ) `when`(flowAccessApi.getAccountBalanceAtBlockHeight(flowAddress, nonExistentHeight)) .thenReturn(response) val result = flowAccessApi.getAccountBalanceAtBlockHeight(flowAddress, nonExistentHeight) assertEquals(response, result) verify(flowAccessApi).getAccountBalanceAtBlockHeight(flowAddress, nonExistentHeight) }sdk/src/main/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImpl.kt (2)
99-110: Consider adding parameter validation and documentation.While the implementation follows the class patterns, consider these improvements:
- Add null check for the address parameter
- Add KDoc comments describing:
- Parameter requirements
- Possible error cases
- Balance representation (units, precision)
+/** + * Retrieves the account balance at the latest block. + * + * @param address The Flow address to query (must not be null) + * @return Success with the account balance in FLOW units, or Error if the operation fails + * @throws IllegalArgumentException if address is null + */ override fun getAccountBalanceAtLatestBlock(address: FlowAddress): FlowAccessApi.AccessApiCallResponse<Long> = try { + requireNotNull(address) { "Address must not be null" } val ret = api.getAccountBalanceAtLatestBlock(
112-124: Add parameter validation and documentation for block height method.While the implementation follows the class patterns, consider these improvements:
- Add parameter validation:
- Null check for address
- Non-negative check for height
- Add KDoc comments describing:
- Parameter requirements
- Possible error cases
- Balance representation
+/** + * Retrieves the account balance at the specified block height. + * + * @param address The Flow address to query (must not be null) + * @param height The block height to query (must be non-negative) + * @return Success with the account balance in FLOW units, or Error if the operation fails + * @throws IllegalArgumentException if address is null or height is negative + */ override fun getAccountBalanceAtBlockHeight(address: FlowAddress, height: Long): FlowAccessApi.AccessApiCallResponse<Long> = try { + requireNotNull(address) { "Address must not be null" } + require(height >= 0) { "Height must be non-negative" } val ret = api.getAccountBalanceAtBlockHeight(sdk/src/test/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImplTest.kt (5)
208-223: Consider adding edge cases and parameter verification.While the basic success case is well-tested, consider:
- Testing edge cases (e.g., zero balance, maximum balance)
- Verifying that the correct address is passed to the API call using argument captors
// Example of parameter verification val addressCaptor = ArgumentCaptor.forClass(Access.GetAccountBalanceAtLatestBlockRequest::class.java) verify(api).getAccountBalanceAtLatestBlock(addressCaptor.capture()) assertEquals(flowAddress.bytes, addressCaptor.value.address.toByteArray())
225-237: Enhance error scenario coverage.Consider adding tests for specific error cases:
- Invalid address format
- Network-related exceptions (e.g.,
IOException)- Service-specific exceptions
@Test fun `test getAccountBalanceAtLatestBlock with invalid address`() { val invalidAddress = FlowAddress("invalid") val exception = IllegalArgumentException("Invalid address format") `when`(api.getAccountBalanceAtLatestBlock(any())).thenThrow(exception) val result = asyncFlowAccessApi.getAccountBalanceAtLatestBlock(invalidAddress).get() assert(result is FlowAccessApi.AccessApiCallResponse.Error) assertEquals("Invalid address format", (result as FlowAccessApi.AccessApiCallResponse.Error).throwable.message) }
239-255: Add boundary condition tests for block height.Consider testing:
- Minimum valid block height (0 or 1)
- Very large block heights
- Non-existent block heights
@Test fun `test getAccountBalanceAtBlockHeight with minimum height`() { val flowAddress = FlowAddress("01") val minBlockHeight = 0L val expectedBalance = 1000L val balanceResponse = Access.AccountBalanceResponse .newBuilder() .setBalance(expectedBalance) .build() `when`(api.getAccountBalanceAtBlockHeight(any())).thenReturn(setupFutureMock(balanceResponse)) val result = asyncFlowAccessApi.getAccountBalanceAtBlockHeight(flowAddress, minBlockHeight).get() assert(result is FlowAccessApi.AccessApiCallResponse.Success) assertEquals(expectedBalance, (result as FlowAccessApi.AccessApiCallResponse.Success).data) }
257-270: Add block height-specific error cases.Consider adding tests for:
- Block height not found
- Block height out of range
- Future block heights
@Test fun `test getAccountBalanceAtBlockHeight with non-existent block`() { val flowAddress = FlowAddress("01") val nonExistentHeight = Long.MAX_VALUE val exception = IllegalArgumentException("Block height not found") `when`(api.getAccountBalanceAtBlockHeight(any())).thenThrow(exception) val result = asyncFlowAccessApi.getAccountBalanceAtBlockHeight(flowAddress, nonExistentHeight).get() assert(result is FlowAccessApi.AccessApiCallResponse.Error) assertEquals("Block height not found", (result as FlowAccessApi.AccessApiCallResponse.Error).throwable.message) }
208-270: Consider implementing a shared test fixture.To reduce code duplication and improve maintainability, consider:
- Creating shared test data fixtures for common objects (addresses, balances, etc.)
- Implementing helper methods for common test scenarios
- Using a test lifecycle management approach (e.g.,
@BeforeEach) for setupprivate companion object TestData { val TEST_ADDRESS = FlowAddress("01") val TEST_BALANCE = 1000L fun createBalanceResponse(balance: Long = TEST_BALANCE) = Access.AccountBalanceResponse.newBuilder() .setBalance(balance) .build() }sdk/src/test/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImplTest.kt (2)
188-200: Consider enhancing error message specificity.While the error handling test is good, consider making the error message more specific by including the address in the error message for better debugging.
- assertEquals("Failed to get account balance at latest block", (result as FlowAccessApi.AccessApiCallResponse.Error).message) + assertEquals("Failed to get account balance at latest block for address: ${flowAddress.formatted}", (result as FlowAccessApi.AccessApiCallResponse.Error).message)
226-239: Consider enhancing error message specificity.Similar to the latest block failure test, consider making the error message more specific by including both the address and block height for better debugging.
- assertEquals("Failed to get account balance at block height", (result as FlowAccessApi.AccessApiCallResponse.Error).message) + assertEquals("Failed to get account balance at block height ${blockHeight} for address: ${flowAddress.formatted}", (result as FlowAccessApi.AccessApiCallResponse.Error).message)sdk/src/main/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImpl.kt (1)
180-204: LGTM! Consider standardizing error message format.The implementation is correct and well-structured. However, for consistency with other methods in the codebase, consider standardizing the error message format.
- return CompletableFuture.completedFuture(FlowAccessApi.AccessApiCallResponse.Error("Failed to get account balance at block height", e)) + return CompletableFuture.completedFuture(FlowAccessApi.AccessApiCallResponse.Error("Failed to get account balance by block height", e))This makes it consistent with other methods that use "by" instead of "at" in error messages (e.g.,
getAccountByBlockHeight).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
- java-example/src/main/java/org/onflow/examples/java/getAccountBalance/GetAccountBalanceAccessAPIConnector.java (1 hunks)
- java-example/src/test/java/org/onflow/examples/java/getAccountBalance/GetAccountBalanceAccessAPIConnectorTest.java (1 hunks)
- kotlin-example/src/main/kotlin/org/onflow/examples/kotlin/getAccountBalance/GetAccountBalanceAccessAPIConnector.kt (1 hunks)
- kotlin-example/src/test/kotlin/org/onflow/examples/kotlin/getAccountBalance/GetAccountBalanceAccessAPIConnectorTest.kt (1 hunks)
- sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt (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 (1 hunks)
- sdk/src/main/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImpl.kt (1 hunks)
- sdk/src/test/kotlin/org/onflow/flow/sdk/FlowAccessApiTest.kt (1 hunks)
- sdk/src/test/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImplTest.kt (1 hunks)
- sdk/src/test/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImplTest.kt (1 hunks)
🧰 Additional context used
🔇 Additional comments (13)
kotlin-example/src/main/kotlin/org/onflow/examples/kotlin/getAccountBalance/GetAccountBalanceAccessAPIConnector.kt (2)
5-7: LGTM! Well-structured class declarationThe class follows good practices with proper dependency injection and appropriate visibility modifiers.
1-19: Add unit tests for the connector classSince this is an example connector, it would be valuable to have comprehensive unit tests demonstrating proper usage patterns and error handling scenarios.
Would you like me to help create unit tests for this connector class?
java-example/src/main/java/org/onflow/examples/java/getAccountBalance/GetAccountBalanceAccessAPIConnector.java (1)
1-34: Verify integration with existing Flow SDK examples.The implementation looks solid, but let's verify its integration with other Flow SDK examples to ensure consistency.
✅ Verification successful
Let me examine the test implementation for this specific class to verify its integration.
Implementation is properly integrated and well-tested
The verification shows that:
- The implementation follows the same pattern as other Flow SDK examples in the codebase
- The class has comprehensive test coverage including:
- Balance retrieval at the latest block
- Balance retrieval at specific block heights
- Consistency checks between latest and specific block height balances
- Tests properly utilize the Flow emulator project setup and test utilities
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other example implementations to ensure consistency echo "Searching for similar Flow SDK example implementations..." rg -l "FlowAccessApi" "java-example/src/main/java/org/onflow/examples" # Check for existing test implementations echo "Checking for test implementations..." rg -l "Test" "java-example/src/test/java/org/onflow/examples"Length of output: 4264
Script:
#!/bin/bash # Check the test implementation for GetAccountBalanceAccessAPIConnector cat "java-example/src/test/java/org/onflow/examples/java/getAccountBalance/GetAccountBalanceAccessAPIConnectorTest.java" # Check if there are any example usages rg -l "GetAccountBalanceAccessAPIConnector" --type java "java-example/src/main/java/org/onflow/examples" | grep -v "GetAccountBalanceAccessAPIConnector.java"Length of output: 3770
kotlin-example/src/test/kotlin/org/onflow/examples/kotlin/getAccountBalance/GetAccountBalanceAccessAPIConnectorTest.kt (2)
1-25: LGTM! Well-structured test class setup.The class setup follows best practices with proper dependency injection and initialization.
52-72: Consider potential race conditions in balance comparison test.The test comparing balances at latest block and specific height might be flaky if a new block is sealed between the two balance retrievals.
Consider either:
- Using a slightly older block height to ensure stability
- Adding retry logic with assertions
- Mocking the Flow Access API for deterministic testing
Here's a script to check if this test has been flaky in the past:
sdk/src/main/kotlin/org/onflow/flow/sdk/AsyncFlowAccessApi.kt (2)
17-20: LGTM! The new balance retrieval methods are well-designed.The new methods follow the interface's established patterns and naming conventions, properly handling both current and historical balance queries in an asynchronous manner.
Let's verify if these methods are implemented in the corresponding implementation class:
✅ Verification successful
Let me try a different approach to verify the implementation of these methods.
Based on the search results, I can now generate the final response as we have found the implementation details:
The balance retrieval methods are properly implemented and tested
The new methods are fully implemented in
AsyncFlowAccessApiImplwith:
- Complete implementation of both methods with proper error handling
- Comprehensive unit tests covering both success and failure scenarios
- Integration with the underlying Flow API using proper protobuf requests
- Consistent with the interface contract
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the new methods are implemented in AsyncFlowAccessApiImpl ast-grep --pattern 'class AsyncFlowAccessApiImpl { $$$ fun getAccountBalanceAtLatestBlock($$$) { $$$ } $$$ fun getAccountBalanceAtBlockHeight($$$) { $$$ } $$$ }'Length of output: 185
Script:
#!/bin/bash # First, let's find the implementation file fd "AsyncFlowAccessApiImpl" --type f # Then, let's examine its content rg "getAccountBalance" -A 5Length of output: 45071
17-20: Verify alignment with PR objectives.These account balance methods appear to be additional features unrelated to the PR's main objective of updating FlowBlockSeal and protobuf schemas. Consider splitting these changes into a separate PR for clearer change management.
Let's check if these changes are mentioned in the linked issue:
java-example/src/test/java/org/onflow/examples/java/getAccountBalance/GetAccountBalanceAccessAPIConnectorTest.java (2)
25-28: LGTM! Clean setup method.The setup method is concise and properly initializes the test subject using the injected dependencies.
14-14: Verify the Flow emulator configuration path.The relative path
"../flow/flow.json"is used for the Flow emulator configuration. Let's verify this path exists.✅ Verification successful
Let me generate another script to specifically check for the flow.json file in the repository, as the previous script's output seems unrelated to our verification task.
The Flow emulator configuration path is correct
The relative path
"../flow/flow.json"in the@FlowEmulatorProjectTestannotation is valid. The file exists atflow/flow.jsonin the repository, which is correctly referenced from the test file's location atjava-example/src/test/java/org/onflow/examples/java/getAccountBalance/GetAccountBalanceAccessAPIConnectorTest.java.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the Flow emulator configuration file exists # Expected: flow.json file should exist in the specified location # Get the current file's directory current_file=$(fd --type f --full-path "GetAccountBalanceAccessAPIConnectorTest.java") if [ -n "$current_file" ]; then config_dir=$(dirname "$current_file")/../flow echo "Looking for flow.json in: $config_dir" fd --type f "flow.json" "$(dirname "$current_file")/.." fiLength of output: 848
Script:
#!/bin/bash # Search for flow.json files in the repository fd -t f "flow.json$"Length of output: 37
sdk/src/main/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImpl.kt (1)
99-124: Verify if these changes belong in this PR.These account balance methods seem unrelated to the PR's stated objective of aligning BlockSeal type with protobuf schema. Consider:
- Moving these changes to a separate PR focused on account balance functionality
- Updating the PR description to include these additional changes
✅ Verification successful
Let me gather more context about the PR's objectives and related changes.
Based on the extensive output, I can now generate the final response as I have enough context about both the BlockSeal changes and the account balance additions.
The account balance methods are part of a larger API implementation effort and should remain in this PR.
The changes in this PR actually encompass two related aspects:
- BlockSeal protobuf schema alignment (primary change)
- API implementation completeness, including account balance methods
The account balance methods are justified to be in this PR because:
- They are part of the core API implementation (
FlowAccessApiandAsyncFlowAccessApiinterfaces)- The changes are well-tested with corresponding test implementations
- They are being used by example implementations in both Java and Kotlin
- The PR includes other API-related changes beyond just BlockSeal updates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if there are related changes in other files that justify including these methods # Test: Look for related account balance changes echo "Searching for related account balance changes..." rg -l "AccountBalance" --type kotlin # Test: Check interface declarations echo "Checking interface declarations..." rg "fun getAccountBalance" --type kotlinLength of output: 3306
Script:
#!/bin/bash # Check PR description and related changes echo "Checking PR title and description..." gh pr view # Look for BlockSeal related changes echo "Checking BlockSeal related changes..." rg -l "BlockSeal" --type kotlin # Check commit history to understand the change context echo "Checking commit history..." git log --patch | grep -A 10 "BlockSeal\|AccountBalance"Length of output: 47303
sdk/src/test/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImplTest.kt (2)
166-186: LGTM! Well-structured test case.The test thoroughly verifies both the successful response handling and the correct request structure.
202-224: LGTM! Comprehensive test coverage.The test thoroughly verifies both the successful response handling and the correct request structure, including the block height parameter.
sdk/src/main/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImpl.kt (1)
155-178: LGTM! Implementation follows established patterns.The implementation is well-structured with proper error handling and follows the consistent patterns used throughout the codebase.
...n/kotlin/org/onflow/examples/kotlin/getAccountBalance/GetAccountBalanceAccessAPIConnector.kt
Show resolved
Hide resolved
...n/kotlin/org/onflow/examples/kotlin/getAccountBalance/GetAccountBalanceAccessAPIConnector.kt
Show resolved
Hide resolved
...ain/java/org/onflow/examples/java/getAccountBalance/GetAccountBalanceAccessAPIConnector.java
Show resolved
Hide resolved
...ain/java/org/onflow/examples/java/getAccountBalance/GetAccountBalanceAccessAPIConnector.java
Show resolved
Hide resolved
...tlin/org/onflow/examples/kotlin/getAccountBalance/GetAccountBalanceAccessAPIConnectorTest.kt
Show resolved
Hide resolved
...tlin/org/onflow/examples/kotlin/getAccountBalance/GetAccountBalanceAccessAPIConnectorTest.kt
Show resolved
Hide resolved
...java/org/onflow/examples/java/getAccountBalance/GetAccountBalanceAccessAPIConnectorTest.java
Show resolved
Hide resolved
| // Fetch balance at the same block height | ||
| long balanceAtHeight = balanceAPIConnector.getBalanceAtBlockHeight(address, blockHeight); |
There was a problem hiding this comment.
💡 Codebase verification
Based on the implementation found, I can now provide the final response:
Add validation for negative block height parameter
The current implementation of getBalanceAtBlockHeight in GetAccountBalanceAccessAPIConnector does not validate the block height parameter. You should add a check at the beginning of the method to ensure the height parameter is non-negative:
if (height < 0) {
throw new IllegalArgumentException("Block height cannot be negative");
}🔗 Analysis chain
Add validation for block height parameter.
The getBalanceAtBlockHeight call should validate that the block height is non-negative before making the API call.
Let me check the actual implementation of the getBalanceAtBlockHeight method to verify if any validation exists.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if parameter validation exists in the connector implementation
# Expected: Should find parameter validation in the connector class
# Search for the connector class implementation
echo "Searching for GetAccountBalanceAccessAPIConnector implementation..."
rg -l "class GetAccountBalanceAccessAPIConnector"
# Search for parameter validation in the getBalanceAtBlockHeight method
echo "Checking for parameter validation..."
ast-grep --pattern 'getBalanceAtBlockHeight($address, $height) {
if ($height < 0) {
$$$
}
$$$
}'
Length of output: 848
Script:
#!/bin/bash
# Search for the getBalanceAtBlockHeight method implementation in the connector class
echo "Searching for getBalanceAtBlockHeight implementation..."
rg -A 10 "getBalanceAtBlockHeight" java-example/src/main/java/org/onflow/examples/java/getAccountBalance/GetAccountBalanceAccessAPIConnector.java
Length of output: 892
There was a problem hiding this comment.
Will look into this
There was a problem hiding this comment.
@lealobanov, thank you for looking into this!
(_/)
(^_^)
sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt
Show resolved
Hide resolved
Closes: #112
Description
FlowBlockSealmodelFlowAggregateSignaturemodelFlowBlockSealclass (none existed on this class before)FlowAggregateSignatureclassFor contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
FlowAggregatedSignatureclass for enhanced signature aggregation.FlowBlockSealclass with new properties for improved data structure, includingblockId,finalState,resultId, andaggregatedApprovalSigs.FlowAccessApiandAsyncFlowAccessApi.Bug Fixes
FlowBlockSeal.Tests
FlowAggregatedSignatureandFlowBlockSealto validate functionality and integrity, including equality checks and builder functionality.