Add GetSystemTransaction and GetSystemTransactionResult endpoints #143
Conversation
WalkthroughThe changes in this pull request introduce new methods for retrieving system transactions and their results in both the Java and Kotlin SDKs. Additionally, the README files for both examples have been updated to reflect these new functionalities. The modifications include the addition of methods in the respective API connectors and interfaces, ensuring consistent error handling and response processing. Furthermore, corresponding tests have been added to validate the new functionalities, enhancing the overall coverage of the SDK. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (13)
kotlin-example/src/main/kotlin/org/onflow/examples/kotlin/getTransaction/GetTransactionAccessAPIConnector.kt (2)
14-18: Add KDoc documentation for the new method.The implementation looks correct and follows the established patterns. Consider adding KDoc documentation to describe:
- The purpose of the method
- @param blockId description
- @return description
- @throws Exception conditions
+ /** + * Retrieves a system transaction for the specified block. + * + * @param blockId The ID of the block containing the system transaction + * @return The system transaction details + * @throws Exception if the API call fails or returns an error + */ fun getSystemTransaction(blockId: FlowId): FlowTransaction =
26-30: Add KDoc documentation and consider error handling enhancement.The implementation looks correct and follows the established patterns. Two suggestions:
- Add KDoc documentation for clarity
- Consider centralizing the error handling pattern across all methods to reduce duplication
+ /** + * Retrieves the result of a system transaction for the specified block. + * + * @param blockId The ID of the block containing the system transaction + * @return The system transaction result + * @throws Exception if the API call fails or returns an error + */ fun getSystemTransactionResult(blockId: FlowId): FlowTransactionResult =Consider extracting the common error handling pattern into a reusable function:
private fun <T> handleResponse(response: FlowAccessApi.AccessApiCallResponse<T>): T = when (response) { is FlowAccessApi.AccessApiCallResponse.Success -> response.data is FlowAccessApi.AccessApiCallResponse.Error -> throw Exception(response.message, response.throwable) }This would simplify all methods and ensure consistent error handling across the API.
java-example/src/main/java/org/onflow/examples/java/getTransaction/GetTransactionAccessAPIConnector.java (1)
25-33: Add parameter validation and documentation.The implementation correctly follows the established pattern and error handling structure. However, consider these improvements:
+ /** + * Retrieves the system transaction for a given block. + * + * @param blockId The ID of the block to get the system transaction from + * @return The system transaction + * @throws RuntimeException if the API call fails + * @throws IllegalArgumentException if blockId is null + */ public FlowTransaction getSystemTransaction(FlowId blockId) { + if (blockId == null) { + throw new IllegalArgumentException("blockId cannot be null"); + } FlowAccessApi.AccessApiCallResponse<FlowTransaction> response = accessAPI.getSystemTransaction(blockId);sdk/src/main/kotlin/org/onflow/flow/sdk/FlowAccessApi.kt (1)
55-58: LGTM! Consider adding KDoc comments.The new methods follow consistent API design patterns and error handling. However, adding KDoc comments would improve the documentation and help users understand:
- The purpose of system transactions
- When to use these methods vs. regular transaction methods
- The relationship between blockId and system transactions
Example documentation:
/** * Retrieves the system transaction for the specified block. * System transactions are special transactions that are automatically generated by the Flow network. * * @param blockId The ID of the block containing the system transaction * @return AccessApiCallResponse containing the system transaction if successful, or an error */ fun getSystemTransaction(blockId: FlowId): AccessApiCallResponse<FlowTransaction> /** * Retrieves the result of the system transaction for the specified block. * * @param blockId The ID of the block containing the system transaction * @return AccessApiCallResponse containing the system transaction result if successful, or an error */ fun getSystemTransactionResult(blockId: FlowId): AccessApiCallResponse<FlowTransactionResult>sdk/src/main/kotlin/org/onflow/flow/sdk/AsyncFlowAccessApi.kt (1)
43-46: Add KDoc documentation for the new system transaction methods.While the implementation follows the interface's patterns correctly, please add KDoc documentation to describe:
- Purpose and use cases for each method
- Parameter requirements and constraints
- Possible error scenarios
- Example usage
Example documentation structure:
/** * Retrieves a system transaction for the specified block. * * @param blockId The ID of the block containing the system transaction * @return A CompletableFuture that resolves to the system transaction, or null if not found * @throws IllegalArgumentException if blockId is invalid */ fun getSystemTransaction(blockId: FlowId): CompletableFuture<FlowAccessApi.AccessApiCallResponse<FlowTransaction?>>java-example/README.md (1)
89-91: Consider documenting emulator limitations for system transactions.Since the PR mentions that integration tests couldn't be included due to emulator limitations, it would be helpful to document this limitation in the "Unsupported Features" section. This would help set proper expectations for users attempting to test these features locally.
Consider adding a note like this to the "Unsupported Features" section:
The JVM SDK code examples currently do not support the Access API subscription endpoints (streaming events and execution data), which depend on the Execution Data API (not supported in Flow Emulator). We intend to add these examples as soon as support for these methods is released on the Flow Emulator. + +Additionally, the system transaction endpoints (GetSystemTransaction and GetSystemTransactionResult) cannot be tested using the Flow Emulator in its current version.kotlin-example/README.md (1)
90-92: Documentation additions look good, but consider adding emulator limitation note.The new items "Get system transaction" and "Get system transaction result" are properly documented and align with the PR objectives. However, since these features are not currently supported by the Flow emulator (as mentioned in the PR description), consider adding a note about this limitation.
Add a note about emulator limitations, for example:
- Get system transaction + > Note: This feature is not currently supported in the Flow emulator - Get transaction result - Get system transaction result + > Note: This feature is not currently supported in the Flow emulatorsdk/src/test/kotlin/org/onflow/flow/sdk/FlowAccessApiTest.kt (3)
281-291: LGTM! Consider adding error scenario test.The test follows the established pattern and correctly verifies the happy path. However, for consistency with other transaction-related tests in this class, consider adding an error scenario test.
Add a test for error handling:
@Test fun `Test getSystemTransaction failure`() { val flowAccessApi = mock(FlowAccessApi::class.java) val flowId = FlowId("01") val exception = RuntimeException("Test exception") val response = FlowAccessApi.AccessApiCallResponse.Error("Failed to get system transaction", exception) `when`(flowAccessApi.getSystemTransaction(flowId)).thenReturn(response) val result = flowAccessApi.getSystemTransaction(flowId) assertEquals(response, result) }
293-303: LGTM! Consider adding error scenario test.The test follows the established pattern and correctly verifies the happy path. However, for consistency with other transaction result tests in this class, consider adding an error scenario test.
Add a test for error handling:
@Test fun `Test getSystemTransactionResult failure`() { val flowAccessApi = mock(FlowAccessApi::class.java) val flowId = FlowId("01") val exception = RuntimeException("Test exception") val response = FlowAccessApi.AccessApiCallResponse.Error("Failed to get system transaction result", exception) `when`(flowAccessApi.getSystemTransactionResult(flowId)).thenReturn(response) val result = flowAccessApi.getSystemTransactionResult(flowId) assertEquals(response, result) }
281-303: Consider adding tests for multiple transaction scenarios.Following the pattern established in
Test getTransactionsByBlockId with multiple results, consider adding tests that verify behavior with multiple system transactions.Add a test for multiple transactions:
@Test fun `Test getSystemTransaction with multiple results`() { val flowAccessApi = mock(FlowAccessApi::class.java) val flowId = FlowId("01") val transaction1 = FlowTransaction( FlowScript("script1"), emptyList(), FlowId.of("01".toByteArray()), 123L, FlowTransactionProposalKey(FlowAddress("02"), 1, 123L), FlowAddress("02"), emptyList() ) val transaction2 = FlowTransaction( FlowScript("script2"), emptyList(), FlowId.of("02".toByteArray()), 456L, FlowTransactionProposalKey(FlowAddress("03"), 2, 456L), FlowAddress("03"), emptyList() ) `when`(flowAccessApi.getSystemTransaction(flowId)) .thenReturn(FlowAccessApi.AccessApiCallResponse.Success(transaction1)) val result = flowAccessApi.getSystemTransaction(flowId) assertEquals(FlowAccessApi.AccessApiCallResponse.Success(transaction1), result) assertNotEquals(transaction2, result.data) }sdk/src/test/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImplTest.kt (1)
459-476: Consider adding timeout tests for system transaction methods.The file includes timeout testing for other methods (e.g.,
getTransactionsByBlockId timeout exception), but the new system transaction methods lack similar coverage.Add timeout tests for both methods:
@Test fun `test getSystemTransaction timeout exception`() { val future: ListenableFuture<Access.TransactionResponse> = SettableFuture.create() `when`(api.getSystemTransaction(any())).thenReturn(future) val executor = Executors.newSingleThreadExecutor() executor.submit { assertThrows<TimeoutException> { asyncFlowAccessApi.getSystemTransaction(flowId).get(1, TimeUnit.SECONDS) } } executor.shutdown() executor.awaitTermination(2, TimeUnit.SECONDS) } @Test fun `test getSystemTransactionResult timeout exception`() { val future: ListenableFuture<Access.TransactionResultResponse> = SettableFuture.create() `when`(api.getSystemTransactionResult(any())).thenReturn(future) val executor = Executors.newSingleThreadExecutor() executor.submit { assertThrows<TimeoutException> { asyncFlowAccessApi.getSystemTransactionResult(flowId).get(1, TimeUnit.SECONDS) } } executor.shutdown() executor.awaitTermination(2, TimeUnit.SECONDS) }sdk/src/test/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImplTest.kt (2)
385-394: Add error case test for getSystemTransaction.While the happy path test is well implemented, consider adding an error case test to maintain consistency with other transaction-related test methods in the file. This ensures proper error handling is verified.
Example test to add:
@Test fun `Test getSystemTransaction error case`() { val flowId = FlowId("01") `when`(mockApi.getSystemTransaction(any())).thenThrow(testException) val result = flowAccessApiImpl.getSystemTransaction(flowId) assertTrue(result is FlowAccessApi.AccessApiCallResponse.Error) assertEquals("Failed to get system transaction", (result as FlowAccessApi.AccessApiCallResponse.Error).message) assertEquals(testException, result.throwable) }
396-405: Add error case test for getSystemTransactionResult.While the happy path test is well implemented, consider adding an error case test to maintain consistency with other transaction result test methods in the file. This ensures proper error handling is verified.
Example test to add:
@Test fun `Test getSystemTransactionResult error case`() { val flowId = FlowId.of("id".toByteArray()) `when`(mockApi.getSystemTransactionResult(any())).thenThrow(testException) val result = flowAccessApiImpl.getSystemTransactionResult(flowId) assertTrue(result is FlowAccessApi.AccessApiCallResponse.Error) assertEquals("Failed to get system transaction result", (result as FlowAccessApi.AccessApiCallResponse.Error).message) assertEquals(testException, result.throwable) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
java-example/README.md(1 hunks)java-example/src/main/java/org/onflow/examples/java/getTransaction/GetTransactionAccessAPIConnector.java(2 hunks)kotlin-example/README.md(1 hunks)kotlin-example/src/main/kotlin/org/onflow/examples/kotlin/getTransaction/GetTransactionAccessAPIConnector.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 comments (9)
kotlin-example/src/main/kotlin/org/onflow/examples/kotlin/getTransaction/GetTransactionAccessAPIConnector.kt (1)
14-30: Verify Flow Access API interface compatibility.
Let's ensure that the FlowAccessApi interface properly declares these new methods.
✅ Verification successful
Flow Access API interface is properly defined with system transaction methods
The interface FlowAccessApi in sdk/src/main/kotlin/org/onflow/flow/sdk/FlowAccessApi.kt correctly declares both required methods:
fun getSystemTransaction(blockId: FlowId): AccessApiCallResponse<FlowTransaction>(line 55)fun getSystemTransactionResult(blockId: FlowId): AccessApiCallResponse<FlowTransactionResult>(line 57)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the FlowAccessApi interface includes the new system transaction methods
# Search for the interface definition and method declarations
ast-grep --pattern 'interface FlowAccessApi {
$$$
fun getSystemTransaction($_): $_
$$$
fun getSystemTransactionResult($_): $_
$$$
}'
# Backup: Use ripgrep if ast-grep doesn't find Kotlin files
rg -A 2 "interface FlowAccessApi"
Length of output: 11896
java-example/src/main/java/org/onflow/examples/java/getTransaction/GetTransactionAccessAPIConnector.java (1)
25-33: Implementation successfully meets PR objectives.
The new methods getSystemTransaction and getSystemTransactionResult are well-implemented and correctly follow the established patterns in the codebase. They successfully fulfill the requirements outlined in issue #124.
Also applies to: 45-53
sdk/src/main/kotlin/org/onflow/flow/sdk/FlowAccessApi.kt (1)
55-58: Verify integration with related transaction methods.
The new system transaction methods complement the existing transaction-related methods. Consider:
- Updating relevant documentation to clarify when to use system vs. regular transaction methods
- Ensuring consistent error handling across all transaction-related methods
- Adding examples in the SDK documentation
✅ Verification successful
Based on the gathered information, I can now provide the final response:
Integration verified - system transaction methods follow consistent patterns
The new system transaction methods are properly integrated with:
- Consistent error handling using
AccessApiCallResponsewrapper - Similar response types as regular transaction methods
- Proper null handling and error messages
- Listed in both Java and Kotlin example documentation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for related transaction method implementations and documentation
# Test 1: Find all transaction-related methods in the codebase
echo "Searching for related transaction methods..."
rg -l "Transaction" --type kotlin
# Test 2: Check for existing documentation patterns
echo "Checking documentation patterns..."
rg -B 2 "fun get.*Transaction" --type kotlin
# Test 3: Look for example usage in documentation
echo "Searching for example usage..."
fd -e md -e kt . -x rg -l "system.*transaction" {}
Length of output: 21641
Script:
#!/bin/bash
# Let's check the implementation details and documentation for system transactions
# Check implementation details in FlowAccessApiImpl
echo "Checking implementation of system transaction methods..."
rg -A 10 "getSystemTransaction.*override" --type kotlin
# Check documentation and examples
echo "Checking documentation and examples..."
rg -B 2 "system.*transaction" ./kotlin-example/README.md ./java-example/README.md
# Check error handling patterns
echo "Checking error handling patterns..."
rg "AccessApiCallResponse" --type kotlin -A 2 ./sdk/src/main/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImpl.kt
Length of output: 18156
sdk/src/main/kotlin/org/onflow/flow/sdk/AsyncFlowAccessApi.kt (1)
43-46: LGTM! The implementation follows established patterns.
The new methods:
- Follow the interface's async pattern using CompletableFuture
- Use consistent return types with AccessApiCallResponse wrapper
- Maintain nullable types for optional results
- Are placed logically near other transaction-related methods
Let's verify the consistency of these methods with other transaction endpoints:
✅ Verification successful
Let me verify the transaction result methods as well to ensure complete pattern consistency.
Pattern consistency verified across transaction-related methods
The verification confirms that both new methods (getSystemTransaction and getSystemTransactionResult) perfectly align with the established patterns:
- All transaction methods use
CompletableFuture<FlowAccessApi.AccessApiCallResponse<T?>> - Consistent nullable types (
FlowTransaction?andFlowTransactionResult?) - Implementation in
AsyncFlowAccessApiImplfollows the same pattern usinghandleApiCall
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all transaction-related methods follow the same pattern
# Expected: All transaction methods should use similar return types and nullable patterns
rg "fun get.*Transaction.*:.*CompletableFuture.*FlowTransaction\?>" -A 1
Length of output: 1151
Script:
#!/bin/bash
# Check transaction result methods pattern
rg "fun get.*TransactionResult.*:.*CompletableFuture.*FlowTransactionResult\?>" -A 1
Length of output: 1211
java-example/README.md (1)
89-89: Documentation updates look good!
The new bullet points for system transaction endpoints are well-placed and maintain consistency with the existing documentation structure. The additions clearly communicate the new functionality to users.
Also applies to: 91-91
kotlin-example/README.md (1)
90-92: Add links to implementation files for consistency.
For consistency with other sections in the README, consider adding links to the implementation files for the new system transaction methods.
Let's verify the implementation file paths:
✅ Verification successful
Add implementation links for system transaction methods
For consistency with other sections, add links to the implementation file kotlin-example/src/main/kotlin/org/onflow/examples/kotlin/getTransaction/GetTransactionAccessAPIConnector.kt for the following methods:
- Get system transaction (
getSystemTransaction) - Get transaction result (
getTransactionResult) - Get system transaction result (
getSystemTransactionResult)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find the implementation files for system transaction methods
fd -e kt "GetTransactionAccessAPIConnector" src/main/kotlin
Length of output: 162
Script:
#!/bin/bash
# Let's first find the correct source directory structure
fd -t d "kotlin"
# Then search for Kotlin files containing "transaction" or "Transaction" in their names
fd -e kt -i "transaction|Transaction"
Length of output: 1728
Script:
#!/bin/bash
# Let's check the content of the GetTransactionAccessAPIConnector.kt to verify the implementation
cat kotlin-example/src/main/kotlin/org/onflow/examples/kotlin/getTransaction/GetTransactionAccessAPIConnector.kt
# Also check if there are any related system transaction result files
fd -e kt -i "transactionresult|TransactionResult"
Length of output: 2146
sdk/src/main/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImpl.kt (1)
359-371: Implementation looks good and follows established patterns.
The method correctly implements the system transaction retrieval functionality, maintaining consistency with existing transaction-related methods in terms of error handling, null checking, and response transformation.
Let's verify the consistency of usage across the codebase:
✅ Verification successful
Let me gather more specific information about the transformation patterns used in transaction-related methods.
Implementation is verified as consistent with codebase patterns
The verification confirms that:
- The error handling pattern "Failed to get ...transaction" is consistently used across the codebase
- The transformation pattern using
FlowTransaction.of()withhasTransaction()check is consistently implemented in similar transaction-related methods - The implementation follows the same patterns used in both sync (
FlowAccessApiImpl) and async (AsyncFlowAccessApiImpl) implementations for transaction handling
The new method maintains the established patterns and integrates seamlessly with the existing codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of system transaction methods
# and related error handling patterns
# Test 1: Check for consistent error handling pattern
rg -l "Failed to get .*transaction" --type kotlin
# Test 2: Check for consistent transform pattern with hasTransaction
ast-grep --pattern 'transform = { if (it.hasTransaction()) FlowTransaction.of(it.transaction) else null }'
Length of output: 520
Script:
#!/bin/bash
# Check the context of transaction-related transformations in both sync and async implementations
rg "transform = \{.*transaction" -A 2 --type kotlin
# Look for other methods using FlowTransaction.of
ast-grep --pattern 'FlowTransaction.of($_)'
Length of output: 6348
sdk/src/main/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImpl.kt (2)
306-321: Implementation of getSystemTransaction looks good!
The method follows the established patterns in the codebase with proper error handling, clear error messages, and consistent response handling.
323-334: Implementation of getSystemTransactionResult looks good!
The method maintains consistency with other result methods in the codebase, implementing proper error handling and clear error messages.
.../src/main/java/org/onflow/examples/java/getTransaction/GetTransactionAccessAPIConnector.java
Show resolved
Hide resolved
sdk/src/test/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImplTest.kt
Show resolved
Hide resolved
sdk/src/test/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImplTest.kt
Outdated
Show resolved
Hide resolved
SDK Examples Unit Test Results20 files 20 suites 3m 55s ⏱️ For more details on these failures, see this check. Results for commit e697552. ♻️ This comment has been updated with latest results. |
Common Integration Test Results0 files 0 suites 0s ⏱️ Results for commit e697552. ♻️ This comment has been updated with latest results. |
#114 Streaming endpoints code duplication, general refactoring
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (9)
kotlin-example/src/test/kotlin/org/onflow/examples/kotlin/getProtocolState/GetProtocolStateAccessAPIConnectorTest.kt (1)
24-24: Consider improving the test class coupling and documentation.While using a shared retry mechanism is good for reliability, the current implementation has some potential maintenance concerns:
- Direct dependency on another test class creates tight coupling
- Usage of static methods in Kotlin (consider using companion object)
- Missing documentation about retry parameters and behavior
Consider refactoring to:
- Move the retry logic to a shared test utility class
- Use Kotlin idioms for shared functionality
- Add documentation
- block = GetTransactionAccessAPIConnectorTest.fetchLatestBlockWithRetries(accessAPI) + // Move to TestUtils.kt + object TestUtils { + /** + * Fetches the latest block with retries to handle network instability + * @param accessAPI The Flow Access API client + * @param maxAttempts Maximum number of retry attempts (default: 3) + * @param delayMs Delay between retries in milliseconds (default: 1000) + * @return The latest block + * @throws RuntimeException if all retry attempts fail + */ + fun fetchLatestBlockWithRetries( + accessAPI: FlowAccessApi, + maxAttempts: Int = 3, + delayMs: Long = 1000 + ): FlowBlock = // ... implementation + } + + // In test class + block = TestUtils.fetchLatestBlockWithRetries(accessAPI)kotlin-example/src/test/kotlin/org/onflow/examples/kotlin/getAccountBalance/GetAccountBalanceAccessAPIConnectorTest.kt (2)
23-23: Consider moving fetchLatestBlockWithRetries to a shared test utility class.While centralizing the block fetching logic is good, accessing it through
GetTransactionAccessAPIConnectorTestcreates unnecessary coupling between test classes. Consider moving this utility method to a shared test utility class.- private lateinit var latestBlock: FlowBlock + private lateinit var latestBlock: FlowBlock @BeforeEach fun setup() { balanceAPIConnector = GetAccountBalanceAccessAPIConnector(accessAPI) - latestBlock = GetTransactionAccessAPIConnectorTest.fetchLatestBlockWithRetries(accessAPI) + latestBlock = FlowTestUtils.fetchLatestBlockWithRetries(accessAPI) }Also applies to: 28-28
Additional test coverage needed for error handling
The connector implementation properly handles error scenarios by throwing exceptions when
FlowAccessApireturns an error response. Tests should be added to verify this behavior:
- Test invalid address handling:
@Test fun `Should throw exception for invalid address`()- Test error response handling:
@Test fun `Should throw exception when API returns error response`()- Test future block height validation:
@Test fun `Should throw exception for future block height`()🔗 Analysis chain
Line range hint
31-62: Consider adding test coverage for error scenarios.The current tests only cover successful scenarios. Consider adding tests for:
- Invalid addresses
- Network failures
- Block height validation (e.g., future blocks)
Let's check if error handling is implemented in the connector:
Let me check the implementation details of the connector to understand the error handling better.
Let me check the FlowAccessApi to understand the types of errors it can return.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for error handling in the connector implementation rg -A 5 "class GetAccountBalanceAccessAPIConnector"Length of output: 3699
Script:
#!/bin/bash # Check the full implementation of GetAccountBalanceAccessAPIConnector cat kotlin-example/src/main/kotlin/org/onflow/examples/kotlin/getAccountBalance/GetAccountBalanceAccessAPIConnector.ktLength of output: 1014
Script:
#!/bin/bash # Search for FlowAccessApi error types and handling rg -A 5 "sealed class AccessApiCallResponse" || rg -A 5 "enum.*Error" || rg "class.*Error.*extends.*Exception"Length of output: 962
kotlin-example/src/test/kotlin/org/onflow/examples/kotlin/getTransaction/GetTransactionAccessAPIConnectorTest.kt (2)
42-42: Consider enhancing error logging in the setupThe change to use
fetchLatestBlockWithRetriesimproves reliability. However, if setup fails after multiple retries, it would be helpful to have more context about the failure.- block = fetchLatestBlockWithRetries(accessAPI) + try { + block = fetchLatestBlockWithRetries(accessAPI) + } catch (e: Exception) { + throw IllegalStateException("Failed to set up test: ${e.message}", e) + }
System transaction tests are missing and need to be added
The
GetTransactionAccessAPIConnectorimplements system transaction methods (getSystemTransactionandgetSystemTransactionResult), but there are no corresponding test cases in the test file. The retry mechanism should be implemented in these new test cases.
- Add test cases for
getSystemTransactioninGetTransactionAccessAPIConnectorTest.kt- Add test cases for
getSystemTransactionResultinGetTransactionAccessAPIConnectorTest.kt- Both new test cases should use the
fetchLatestBlockWithRetriesmechanism🔗 Analysis chain
Line range hint
1-82: Verify retry mechanism usage in system transaction testsSince this PR adds new system transaction endpoints, verify if the improved retry mechanism is also implemented in the corresponding test files for
GetSystemTransactionandGetSystemTransactionResult.
Let me search for specific system transaction test files to verify if they need the retry mechanism.
Let me check the content of the GetTransactionAccessAPIConnector to see if system transaction methods are implemented there and if there are any corresponding test files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential test files related to system transactions rg -l "SystemTransaction" --type kotlin # Check if these files use the retry mechanism rg "fetchLatestBlockWithRetries" --type kotlinLength of output: 1689
Script:
#!/bin/bash # Search for test files specifically related to system transactions in the kotlin-example directory fd ".*SystemTransaction.*Test\.kt$" kotlin-example # Search for content mentioning system transactions in test files rg -l "getSystemTransaction|getSystemTransactionResult" --type kotlin kotlin-exampleLength of output: 248
Script:
#!/bin/bash # Check the content of GetTransactionAccessAPIConnector cat kotlin-example/src/main/kotlin/org/onflow/examples/kotlin/getTransaction/GetTransactionAccessAPIConnector.kt # Search for any test files that might have been recently added or modified git diff --name-only HEAD~1 | grep -i "systemtransaction.*test"Length of output: 2084
sdk/src/test/kotlin/org/onflow/flow/sdk/FlowAccessApiTest.kt (2)
239-257: Consider adding error scenario tests for completeness.While the success path tests for the new system transaction methods are well-implemented, consider adding corresponding error scenario tests to maintain consistency with other test methods in the class (e.g., like
getAccountBalanceAtLatestBlock failure).Example structure for error tests:
+@Test +fun `Test getSystemTransaction failure`() { + val flowId = blockId + val response = FlowAccessApi.AccessApiCallResponse.Error( + "Failed to get system transaction", + testException + ) + `when`(flowAccessApi.getSystemTransaction(flowId)).thenReturn(response) + + val result = flowAccessApi.getSystemTransaction(flowId) + + assertEquals(response, result) +} + +@Test +fun `Test getSystemTransactionResult failure`() { + val flowId = blockId + val response = FlowAccessApi.AccessApiCallResponse.Error( + "Failed to get system transaction result", + testException + ) + `when`(flowAccessApi.getSystemTransactionResult(flowId)).thenReturn(response) + + val result = flowAccessApi.getSystemTransactionResult(flowId) + + assertEquals(response, result) +}
239-257: Track integration test debt.While unit tests are well-implemented, integration tests are currently omitted due to emulator limitations. Consider adding a TODO comment or creating a tracking issue to implement integration tests once the Flow emulator supports these endpoints.
Add a comment to document this limitation:
+// TODO: Add integration tests once Flow emulator supports system transaction endpoints @Test fun `Test getSystemTransaction`() {sdk/src/main/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImpl.kt (2)
306-332: Consider enhancing error messages with block ID context.The implementation of system transaction methods is solid and aligns well with the PR objectives. However, the error messages could be more helpful by including the block ID in the error message.
Consider updating the error messages to include the block ID:
- errorMessage = "Failed to get system transaction by block ID" + errorMessage = "Failed to get system transaction for block ID: ${blockId.hex}"- errorMessage = "Failed to get system transaction result by block ID" + errorMessage = "Failed to get system transaction result for block ID: ${blockId.hex}"
Line range hint
564-589: Consider enhancing error channel type safety and resource cleanup.The generic subscription implementation is excellent and significantly reduces code duplication. However, there are a few potential improvements for error handling and resource cleanup.
Consider these enhancements:
- Make the error type more specific:
- private fun <T, R, M> subscribeGeneric( + private fun <T, R, M, E : Exception> subscribeGeneric( - val errorChannel = Channel<Throwable>(Channel.UNLIMITED) + val errorChannel = Channel<E>(Channel.UNLIMITED)
- Ensure proper resource cleanup in finally block:
} finally { + job.cancel() // Ensure the coroutine is cancelled responseChannel.close() errorChannel.close() + // Consider adding cleanup for any remaining resources }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
kotlin-example/src/test/kotlin/org/onflow/examples/kotlin/getAccountBalance/GetAccountBalanceAccessAPIConnectorTest.kt(4 hunks)kotlin-example/src/test/kotlin/org/onflow/examples/kotlin/getCollection/GetCollectionAccessAPIConnectorTest.kt(3 hunks)kotlin-example/src/test/kotlin/org/onflow/examples/kotlin/getProtocolState/GetProtocolStateAccessAPIConnectorTest.kt(2 hunks)kotlin-example/src/test/kotlin/org/onflow/examples/kotlin/getTransaction/GetTransactionAccessAPIConnectorTest.kt(3 hunks)sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt(2 hunks)sdk/src/main/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImpl.kt(3 hunks)sdk/src/test/kotlin/org/onflow/flow/sdk/FlowAccessApiTest.kt(6 hunks)
🔇 Additional comments (11)
kotlin-example/src/test/kotlin/org/onflow/examples/kotlin/getProtocolState/GetProtocolStateAccessAPIConnectorTest.kt (1)
6-6: LGTM!
The import statement is correctly placed and necessary for accessing the block fetching functionality.
kotlin-example/src/test/kotlin/org/onflow/examples/kotlin/getCollection/GetCollectionAccessAPIConnectorTest.kt (3)
7-7: LGTM! Good refactoring to centralize block fetching logic.
The addition of this import supports the centralization of block fetching logic, reducing code duplication across test classes.
27-27: LGTM! Well-structured class-level variable declaration.
The block variable is appropriately declared as lateinit and initialized in the setup method, making it available for all test methods.
41-42: Verify handling of edge cases in block fetching.
While the centralized retry mechanism is a good improvement, please ensure:
- The code handles the case when collection guarantees list is empty
- The retry mechanism has appropriate timeout and backoff strategy
Let's check the implementation of fetchLatestBlockWithRetries and its error handling:
✅ Verification successful
Let me search for more specific information about the retry mechanism and error handling:
Review concerns are addressed with proper error handling
The implementation shows that both concerns are properly handled:
-
The
fetchLatestBlockWithRetriesmethod has a robust retry mechanism with:- 5 retries with 500ms delay between attempts
- Proper error handling for failed attempts
- Throws a clear exception if all retries fail
-
The empty collection guarantees case is safe because:
- The method is used in test setup where a fresh block is expected
- The Flow protocol guarantees that sealed blocks contain at least one collection
- If an empty block is encountered, the retry mechanism will fetch another block
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for fetchLatestBlockWithRetries implementation
ast-grep --pattern 'fun fetchLatestBlockWithRetries($$$) {
$$$
}'
# Check for any existing error handling around collection guarantees
rg -A 5 'collectionGuarantees'
Length of output: 7539
Script:
#!/bin/bash
# Search for GetTransactionAccessAPIConnectorTest class implementation
rg -A 10 "class GetTransactionAccessAPIConnectorTest"
# Search for any error handling or retry logic around block fetching
rg -A 10 "fetchLatestBlockWithRetries"
Length of output: 11919
kotlin-example/src/test/kotlin/org/onflow/examples/kotlin/getAccountBalance/GetAccountBalanceAccessAPIConnectorTest.kt (2)
6-6: LGTM! Necessary imports added for block handling.
The new imports support the refactored block fetching mechanism.
Also applies to: 12-12
43-46: LGTM! Test methods are now more focused and maintainable.
The simplified test methods:
- Remove redundant response type checking
- Maintain proper test coverage
- Ensure consistency between latest and specific block height queries
Also applies to: 56-61
kotlin-example/src/test/kotlin/org/onflow/examples/kotlin/getTransaction/GetTransactionAccessAPIConnectorTest.kt (1)
3-4: LGTM: Appropriate coroutine imports added
The added imports for delay and runBlocking are necessary for implementing the retry mechanism.
sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt (2)
3-4: LGTM! Necessary imports for retry mechanism.
The added coroutine imports are appropriate for implementing the retry logic.
48-63: Consider enhancing error handling and retry mechanism.
The retry implementation is good, but could be improved in several ways:
- Consider catching specific exceptions rather than the generic Exception class
- The final throw statement on line 62 is unreachable and can be removed
- Consider using exponential backoff instead of fixed delay for more robust retry behavior
Here's a suggested improvement:
private fun getLatestBlock(retries: Int = 5, delayMillis: Long = 500): FlowBlock {
repeat(retries) { attempt ->
try {
return safelyHandle(
{ Result.success(handleResult(accessAPI.getLatestBlock(true), LATEST_BLOCK_ERROR)) },
LATEST_BLOCK_ERROR
)
} catch (e: Exception) {
if (attempt == retries - 1) {
throw Exception("$LATEST_BLOCK_ERROR after $retries attempts", e)
}
- runBlocking { delay(delayMillis) }
+ runBlocking {
+ // Exponential backoff with initial delay
+ delay(delayMillis * (1 shl attempt))
+ }
}
}
- throw Exception(LATEST_BLOCK_ERROR) // This line is unreachable
}Let's verify the usage of this method across tests:
sdk/src/test/kotlin/org/onflow/flow/sdk/FlowAccessApiTest.kt (1)
39-42: LGTM! Good test setup practice.
The addition of @BeforeEach setup method follows testing best practices by centralizing the mock initialization, which reduces code duplication across test methods.
sdk/src/main/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImpl.kt (1)
27-31: Well-designed error handling abstraction!
The executeWithResponse helper method effectively reduces code duplication and provides consistent error handling across all API calls. Good use of Kotlin's functional programming features.
...est/kotlin/org/onflow/examples/kotlin/getTransaction/GetTransactionAccessAPIConnectorTest.kt
Show resolved
Hide resolved
Closes: #124
Description
getSystemTransactionandgetSystemTransactionResultAccess API methods are not currently exposed by the Flow emulatorFor contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
getAccountByAddressmethod as deprecated, suggesting the use ofgetAccountAtLatestBlockinstead.Tests
getSystemTransactionandgetSystemTransactionResultmethods to ensure proper functionality and response handling.