Get account key endpoints #136
Conversation
WalkthroughA new Java class Changes
Assessment against linked issues
Possibly related 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 Results41 files 41 suites 7m 49s ⏱️ For more details on these failures, see this check. Results for commit 0a8e1c9. ♻️ This comment has been updated with latest results. |
Common Integration Test Results0 files 0 suites 0s ⏱️ Results for commit 0a8e1c9. ♻️ 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: 16
🧹 Outside diff range and nitpick comments (24)
kotlin-example/src/main/kotlin/org/onflow/examples/kotlin/getAccountKeys/GetAccountKeysAccessAPIConnector.kt (2)
1-7: Consider using specific imports instead of wildcard import.Replace the wildcard import with specific imports for better code clarity and to avoid potential naming conflicts.
-import org.onflow.flow.sdk.* +import org.onflow.flow.sdk.FlowAccessApi +import org.onflow.flow.sdk.FlowAddress +import org.onflow.flow.sdk.FlowAccountKey
8-30: Add KDoc documentation for public methods.Add documentation to describe method parameters, return values, and possible exceptions for better API understanding.
Example for one method:
/** * Retrieves an account key at the latest block. * * @param address The Flow address of the account * @param keyIndex The index of the key to retrieve * @return The account key at the specified index * @throws FlowKeyNotFoundException if the key is not found * @throws FlowApiException if the API call fails */ fun getAccountKeyAtLatestBlock(address: FlowAddress, keyIndex: Int): FlowAccountKeysdk/src/main/kotlin/org/onflow/flow/sdk/AsyncFlowAccessApi.kt (1)
13-16: LGTM! Methods provide comprehensive account key access.Both methods follow consistent patterns and complement the single-key retrieval methods. The return types correctly handle multiple keys, and the parameter naming is clear and consistent.
Consider implementing caching for these methods in the implementation class, especially for the latest block queries, to improve performance for frequent requests.
sdk/src/main/kotlin/org/onflow/flow/sdk/FlowAccessApi.kt (1)
21-28: Consider adding KDoc documentation.While the method signatures are clear, adding KDoc documentation would help users understand:
- Parameter constraints (e.g., valid key index ranges)
- Error conditions that might occur
- Example usage scenarios
Example documentation format:
/** * Retrieves an account key at the latest sealed block. * * @param address The Flow address to query * @param keyIndex The index of the key to retrieve (must be non-negative) * @return Success with FlowAccountKey if found, Error if key index is invalid or account not found */ fun getAccountKeyAtLatestBlock(address: FlowAddress, keyIndex: Int): AccessApiCallResponse<FlowAccountKey>kotlin-example/src/test/kotlin/org/onflow/examples/kotlin/getAccountKeys/GetAccountKeysAccessAPIConnectorTest.kt (1)
12-25: Add documentation for test environment setup requirements.Consider adding KDoc comments to explain:
- Required Flow emulator configuration
- Purpose of the service account credentials
- Test client configuration requirements
Example addition:
/** * Tests for GetAccountKeysAccessAPIConnector using Flow emulator. * Requires: * - Flow emulator running with configuration from "../flow/flow.json" * - Service account credentials configured in the emulator * - Test client with appropriate network access */ @FlowEmulatorProjectTest(flowJsonLocation = "../flow/flow.json") internal class GetAccountKeysAccessAPIConnectorTest {java-example/src/test/java/org/onflow/examples/java/getAccountKeys/GetAccountKeysAccessAPIConnectorTest.java (1)
32-113: Consider adding edge case tests.The current test suite covers the happy path well, but consider adding tests for:
- Invalid key indices
- Non-existent addresses
- Invalid block heights
- Error responses from the API
Would you like me to help generate these additional test cases?
sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt (2)
58-74: Enhance test coverage with edge cases and additional assertions.While the basic happy path is covered, consider adding:
- Test cases for invalid key indices
- Additional assertions for other key properties (e.g., publicKey, weight, revoked)
- Verification of the key's signing algorithm and hashing algorithm
Example additions:
// Test invalid key index @Test fun `Get account key at latest block with invalid index throws exception`() { val address = serviceAccount.flowAddress val invalidKeyIndex = 999 assertThrows<Exception> { handleResult( accessAPI.getAccountKeyAtLatestBlock(address, invalidKeyIndex), "Should fail with invalid key index" ) } }
103-118: Strengthen assertions for account keys collection.The current assertions only verify that the collection is non-null and non-empty. Consider:
- Verifying the expected number of keys
- Validating properties of individual keys in the collection
- Checking key ordering by sequence number
Example improvements:
assertThat(accountKeys).isNotNull assertThat(accountKeys).isNotEmpty // Add these assertions: assertThat(accountKeys).hasSize(1) // Adjust size based on expected number of keys assertThat(accountKeys).isSortedAccordingTo { a, b -> a.sequenceNumber.compareTo(b.sequenceNumber) } assertThat(accountKeys[0].revoked).isFalse()sdk/src/test/kotlin/org/onflow/flow/sdk/FlowAccessApiTest.kt (2)
21-34: Add error scenario test for getAccountKeyAtLatestBlock.The test only covers the success scenario. Consider adding tests for error cases such as invalid address or key index.
Add a test like this:
@Test fun `Test getAccountKeyAtLatestBlock error`() { val flowAccessApi = mock(FlowAccessApi::class.java) val address = FlowAddress("01") val keyIndex = 0 val exception = RuntimeException("Key not found") `when`(flowAccessApi.getAccountKeyAtLatestBlock(address, keyIndex)) .thenReturn(FlowAccessApi.AccessApiCallResponse.Error("Failed to get account key", exception)) val result = flowAccessApi.getAccountKeyAtLatestBlock(address, keyIndex) assertTrue(result is FlowAccessApi.AccessApiCallResponse.Error) verify(flowAccessApi).getAccountKeyAtLatestBlock(address, keyIndex) }
52-67: Enhance test data for getAccountKeysAtLatestBlock.The test uses default instances for
AccountKey, which doesn't validate actual key properties.Consider using more realistic test data:
val accountKeys = listOf( FlowAccountKey.of( AccountOuterClass.AccountKey.newBuilder() .setIndex(0) .setPublicKey(ByteString.copyFrom("testKey1".toByteArray())) .setSignAlgo(SignatureAlgorithm.ECDSA_P256.value) .setHashAlgo(HashAlgorithm.SHA2_256.value) .setWeight(1000) .build() ), FlowAccountKey.of( AccountOuterClass.AccountKey.newBuilder() .setIndex(1) .setPublicKey(ByteString.copyFrom("testKey2".toByteArray())) .setSignAlgo(SignatureAlgorithm.ECDSA_P256.value) .setHashAlgo(HashAlgorithm.SHA2_256.value) .setWeight(500) .build() ) )sdk/src/main/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImpl.kt (3)
53-66: Consider adding height parameter validation.The implementation is correct, but consider validating that the height parameter is non-negative before making the API call.
override fun getAccountKeyAtBlockHeight(address: FlowAddress, keyIndex: Int, height: Long): FlowAccessApi.AccessApiCallResponse<FlowAccountKey> = try { + require(height >= 0) { "Block height must be non-negative" } val ret = api.getAccountKeyAtBlockHeight(
81-93: Consider adding height parameter validation.The implementation is correct, but like the single key retrieval method, consider validating the height parameter.
override fun getAccountKeysAtBlockHeight(address: FlowAddress, height: Long): FlowAccessApi.AccessApiCallResponse<List<FlowAccountKey>> = try { + require(height >= 0) { "Block height must be non-negative" } val ret = api.getAccountKeysAtBlockHeight(
39-93: Well-structured implementation with consistent patterns.The new account key retrieval methods are well-integrated into the codebase, following established patterns for:
- Error handling using
AccessApiCallResponse- Protocol buffer request building
- Type conversion from protocol buffer objects
- Exception handling with descriptive error messages
This consistency makes the code maintainable and easy to understand.
sdk/src/test/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImplTest.kt (4)
79-110: Consider enhancing test coverage with non-default account key data.While the tests are well-structured, using
AccountKey.getDefaultInstance()might not provide comprehensive coverage. Consider adding test cases with various account key configurations (e.g., different weights, signing algorithms, etc.).val mockAccountKey = FlowAccountKey.of( AccountOuterClass.AccountKey.newBuilder() .setIndex(0) .setWeight(1000) .setSequenceNumber(1) .setPublicKey(ByteString.copyFromUtf8("test-public-key")) .build() )
112-145: Consider adding edge case tests for block height.The tests cover basic success and failure scenarios. Consider adding test cases for:
- Block height 0 (genesis block)
- Very large block heights
- Invalid block heights (negative values)
147-215: Enhance list handling test coverage.The tests verify basic list functionality. Consider adding test cases for:
- Empty list of account keys
- Large number of account keys
- Keys with different properties (revoked, hashed, etc.)
// Example test data for various scenarios val mockAccountKeys = listOf( FlowAccountKey.of(AccountOuterClass.AccountKey.newBuilder() .setIndex(0) .setRevoked(true) .build()), FlowAccountKey.of(AccountOuterClass.AccountKey.newBuilder() .setIndex(1) .setHashAlgo(HashAlgorithm.SHA2_256.ordinal) .build()), // Add more variations... )
Line range hint
1-24: Consider organizing tests using nested classes.The test file is well-structured but could be more organized. Consider using nested classes to group related tests:
class AsyncFlowAccessApiImplTest { private val api = mock(AccessAPIGrpc.AccessAPIFutureStub::class.java) private val asyncFlowAccessApi = AsyncFlowAccessApiImpl(api) @Nested inner class AccountKeyTests { // All account key related tests } @Nested inner class AccountTests { // All account related tests } @Nested inner class TransactionTests { // All transaction related tests } }sdk/src/test/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImplTest.kt (3)
77-114: Consider enhancing test data with realistic account key properties.While the tests are well-structured, they use default instance values for
AccountKey. Consider using more realistic test data that includes actual key properties (e.g., public key, signing algorithm, weight) to ensure the serialization/deserialization handles real-world scenarios correctly.Example enhancement:
- val mockAccountKey = FlowAccountKey.of(AccountOuterClass.AccountKey.getDefaultInstance()) + val mockAccountKey = FlowAccountKey.of( + AccountOuterClass.AccountKey.newBuilder() + .setPublicKey(ByteString.copyFrom("mock-public-key".toByteArray())) + .setSignAlgo(SignatureAlgorithm.ECDSA_P256.value) + .setHashAlgo(HashAlgorithm.SHA3_256.value) + .setWeight(1000) + .build() + )
116-156: Add boundary test cases for block height parameter.Consider adding test cases for edge cases:
- Block height = 0 (genesis block)
- Very large block height values
- Invalid block heights (negative values)
Would you like me to help generate these additional test cases?
158-195: Add test case for empty account keys list.The current tests verify the handling of multiple keys, but it would be beneficial to explicitly test the scenario where an account has no keys.
+ @Test + fun `Test getAccountKeysAtLatestBlock with empty keys list`() { + val flowAddress = FlowAddress("01") + val response = Access.AccountKeysResponse.newBuilder().build() + + `when`(mockApi.getAccountKeysAtLatestBlock(any())).thenReturn(response) + + val result = flowAccessApiImpl.getAccountKeysAtLatestBlock(flowAddress) + assertResultSuccess(result) { assertTrue(it.isEmpty()) } + }sdk/src/main/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImpl.kt (2)
46-70: LGTM! Consider standardizing error message format.The implementation is correct and follows the established patterns. However, for better consistency across the API, consider standardizing the error message format.
Apply this change to align with other error messages in the codebase:
- return CompletableFuture.completedFuture(FlowAccessApi.AccessApiCallResponse.Error("Failed to get account key at latest block", e)) + return CompletableFuture.completedFuture(FlowAccessApi.AccessApiCallResponse.Error("Failed to get account key", e))
99-148: Consider adding batch size limit for large key sets.The implementation correctly handles multiple account keys retrieval. However, for accounts with a large number of keys, consider adding a batch size limit to prevent potential memory issues or timeout problems.
Consider implementing one of these approaches:
- Add a parameter to limit the number of keys returned
- Implement pagination for large key sets
- Add a configuration option for maximum keys per request
Would you like me to provide a detailed implementation for any of these approaches?
java-example/src/main/java/org/onflow/examples/java/getAccountKeys/GetAccountKeysAccessAPIConnector.java (2)
1-1: Add a license header to the source fileIt is a good practice to include a license header at the top of source files to specify the terms under which the code is distributed.
For example, you can add:
/* * This source code is licensed under the MIT license found in the * LICENSE file in the root directory of this source tree. */
3-6: Optimize imports by removing unused onesThe import statement for
org.onflow.flow.sdk.FlowAccountKeyis necessary, but ensure that all imported classes are actually used in the code. If there are any unused imports, consider removing them for cleaner code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
- java-example/src/main/java/org/onflow/examples/java/getAccountKeys/GetAccountKeysAccessAPIConnector.java (1 hunks)
- java-example/src/test/java/org/onflow/examples/java/getAccountKeys/GetAccountKeysAccessAPIConnectorTest.java (1 hunks)
- kotlin-example/src/main/kotlin/org/onflow/examples/kotlin/getAccountKeys/GetAccountKeysAccessAPIConnector.kt (1 hunks)
- kotlin-example/src/test/kotlin/org/onflow/examples/kotlin/getAccountKeys/GetAccountKeysAccessAPIConnectorTest.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 (2 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 (2 hunks)
- sdk/src/test/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImplTest.kt (1 hunks)
🔇 Additional comments (13)
kotlin-example/src/main/kotlin/org/onflow/examples/kotlin/getAccountKeys/GetAccountKeysAccessAPIConnector.kt (1)
1-31: Verify test coverage for error scenarios.While the implementation looks good, ensure comprehensive test coverage for various error scenarios:
- Invalid key indices
- Non-existent accounts
- Network failures
- API errors
sdk/src/main/kotlin/org/onflow/flow/sdk/AsyncFlowAccessApi.kt (2)
9-10: LGTM! Method signature follows established patterns.The method signature is well-designed with clear parameter names and appropriate return type for async operation.
11-12: Verify height parameter validation.The method signature looks good. However, ensure that the implementation validates the height parameter to prevent requests for non-existent blocks.
sdk/src/main/kotlin/org/onflow/flow/sdk/FlowAccessApi.kt (2)
21-28: LGTM! Well-structured method signatures.The new account key methods are well-designed with:
- Consistent naming patterns matching existing API methods
- Appropriate parameter types for addresses, key indices, and block heights
- Clear separation between single and multiple key retrieval
- Proper error handling through AccessApiCallResponse wrapper
21-28: Implementation successfully meets PR objectives.The added methods fully satisfy the requirements from issue #126:
- ✓ GetAccountKeyAtLatestBlock
- ✓ GetAccountKeysAtLatestBlock
- ✓ GetAccountKeyAtBlockHeight
- ✓ GetAccountKeysAtBlockHeight
kotlin-example/src/test/kotlin/org/onflow/examples/kotlin/getAccountKeys/GetAccountKeysAccessAPIConnectorTest.kt (1)
1-103: Verify test coverage against all API methods.The test suite covers the core functionality, but let's verify coverage of all API methods mentioned in the PR objectives:
✅ Verification successful
Test coverage is comprehensive and matches all API methods
The verification shows complete test coverage:
All 4 API methods are implemented in both sync (
FlowAccessApiImpl) and async (AsyncFlowAccessApiImpl) variants:
GetAccountKeyAtLatestBlockGetAccountKeyAtBlockHeightGetAccountKeysAtLatestBlockGetAccountKeysAtBlockHeightEach method has corresponding test coverage in:
- Integration tests (
TransactionIntegrationTest)- Example tests (
GetAccountKeysAccessAPIConnectorTest)- Implementation tests (
FlowAccessApiImplTest)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if all required API methods are tested echo "Verifying test coverage for all API methods..." # Search for API method declarations echo "API Method Declarations:" rg "GetAccountKey.*Block|GetAccountKeys.*Block" --type kotlin # Search for corresponding test methods echo "Test Method Coverage:" rg "fun.*account key.*block" --type kotlinLength of output: 3063
java-example/src/test/java/org/onflow/examples/java/getAccountKeys/GetAccountKeysAccessAPIConnectorTest.java (3)
1-26: LGTM! Well-structured test class setup.The test class is properly configured with:
- Appropriate test infrastructure using JUnit 5
- Correct dependency injection for Flow SDK components
- Clear separation of concerns
27-30: LGTM! Clean and focused setup method.The setup method follows best practices by:
- Using @beforeeach for proper test isolation
- Minimal and focused initialization
32-113: LGTM! Comprehensive test coverage for new endpoints.The test suite effectively covers all four new methods mentioned in the PR objectives:
- GetAccountKeyAtLatestBlock
- GetAccountKeysAtLatestBlock
- GetAccountKeyAtBlockHeight
- GetAccountKeysAtBlockHeight
Each method is tested for both successful execution and proper data validation.
sdk/src/main/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImpl.kt (2)
39-51: LGTM! Implementation follows established patterns.The method correctly implements the account key retrieval at latest block with proper error handling and type conversion.
68-79: LGTM! Proper list handling implementation.The method correctly implements bulk account keys retrieval with proper error handling and list conversion.
sdk/src/test/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImplTest.kt (1)
197-237: LGTM! Tests are comprehensive and well-structured.The test cases for retrieving account keys at block height are thorough, covering both success and error scenarios. They maintain consistency with other test methods and align well with the PR objectives.
sdk/src/main/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImpl.kt (1)
72-97: LGTM! Implementation follows best practices.The method correctly implements the retrieval of account keys at a specific block height with proper error handling and parameter validation.
...rc/main/kotlin/org/onflow/examples/kotlin/getAccountKeys/GetAccountKeysAccessAPIConnector.kt
Show resolved
Hide resolved
...est/kotlin/org/onflow/examples/kotlin/getAccountKeys/GetAccountKeysAccessAPIConnectorTest.kt
Show resolved
Hide resolved
...est/kotlin/org/onflow/examples/kotlin/getAccountKeys/GetAccountKeysAccessAPIConnectorTest.kt
Show resolved
Hide resolved
...est/kotlin/org/onflow/examples/kotlin/getAccountKeys/GetAccountKeysAccessAPIConnectorTest.kt
Show resolved
Hide resolved
...est/kotlin/org/onflow/examples/kotlin/getAccountKeys/GetAccountKeysAccessAPIConnectorTest.kt
Show resolved
Hide resolved
| @Test | ||
| fun `Can get account key at latest block`() { | ||
| val address = serviceAccount.flowAddress | ||
| val keyIndex = 0 | ||
|
|
||
| val accountKey = try { | ||
| handleResult( | ||
| accessAPI.getAccountKeyAtLatestBlock(address, keyIndex), | ||
| "Failed to get account key at latest block" | ||
| ) | ||
| } catch (e: Exception) { | ||
| fail("Failed to retrieve account key at latest block: ${e.message}") | ||
| } | ||
|
|
||
| assertThat(accountKey).isNotNull | ||
| assertThat(accountKey.sequenceNumber).isEqualTo(keyIndex) | ||
| } | ||
|
|
||
| @Test | ||
| fun `Can get account key at block height`() { | ||
| val address = serviceAccount.flowAddress | ||
| val keyIndex = 0 | ||
|
|
||
| val latestBlock = try { | ||
| handleResult( | ||
| accessAPI.getLatestBlock(true), | ||
| "Failed to get latest block" | ||
| ) | ||
| } catch (e: Exception) { | ||
| fail("Failed to retrieve latest block: ${e.message}") | ||
| } | ||
|
|
||
| val accountKey = try { | ||
| handleResult( | ||
| accessAPI.getAccountKeyAtBlockHeight(address, keyIndex, latestBlock.height), | ||
| "Failed to get account key at block height" | ||
| ) | ||
| } catch (e: Exception) { | ||
| fail("Failed to retrieve account key at block height: ${e.message}") | ||
| } | ||
|
|
||
| assertThat(accountKey).isNotNull | ||
| assertThat(accountKey.sequenceNumber).isEqualTo(keyIndex) | ||
| } | ||
|
|
||
| @Test | ||
| fun `Can get account keys at latest block`() { | ||
| val address = serviceAccount.flowAddress | ||
|
|
||
| val accountKeys = try { | ||
| handleResult( | ||
| accessAPI.getAccountKeysAtLatestBlock(address), | ||
| "Failed to get account keys at latest block" | ||
| ) | ||
| } catch (e: Exception) { | ||
| fail("Failed to retrieve account keys at latest block: ${e.message}") | ||
| } | ||
|
|
||
| assertThat(accountKeys).isNotNull | ||
| assertThat(accountKeys).isNotEmpty | ||
| } | ||
|
|
||
| @Test | ||
| fun `Can get account keys at block height`() { | ||
| val address = serviceAccount.flowAddress | ||
|
|
||
| val latestBlock = try { | ||
| handleResult( | ||
| accessAPI.getLatestBlock(true), | ||
| "Failed to get latest block" | ||
| ) | ||
| } catch (e: Exception) { | ||
| fail("Failed to retrieve latest block: ${e.message}") | ||
| } | ||
|
|
||
| val accountKeys = try { | ||
| handleResult( | ||
| accessAPI.getAccountKeysAtBlockHeight(address, latestBlock.height), | ||
| "Failed to get account keys at block height" | ||
| ) | ||
| } catch (e: Exception) { | ||
| fail("Failed to retrieve account keys at block height: ${e.message}") | ||
| } | ||
|
|
||
| assertThat(accountKeys).isNotNull | ||
| assertThat(accountKeys).isNotEmpty | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider structural improvements for better maintainability.
The new tests follow good patterns but could benefit from:
- Extracting common setup code to reduce duplication
- Adding constants for shared values (e.g., keyIndex)
- Adding documentation to describe test scenarios and expected behavior
Consider these structural improvements:
class TransactionIntegrationTest {
companion object {
private const val DEFAULT_KEY_INDEX = 0
}
// Helper method for key retrieval
private fun getLatestBlockAndKey(address: FlowAddress, keyIndex: Int = DEFAULT_KEY_INDEX): Pair<FlowBlock, FlowAccountKey> {
val latestBlock = handleResult(
accessAPI.getLatestBlock(true),
"Failed to get latest block"
)
val accountKey = handleResult(
accessAPI.getAccountKeyAtLatestBlock(address, keyIndex),
"Failed to get account key"
)
return latestBlock to accountKey
}
// Tests can then be simplified...
}There was a problem hiding this comment.
Looking into this
There was a problem hiding this comment.
@lealobanov, thank you for looking into this!
(_/)
( ^^)
(")(")
| try { | ||
| api.getAccountKeyAtLatestBlock( | ||
| Access.GetAccountKeyAtLatestBlockRequest | ||
| .newBuilder() | ||
| .setAddress(address.byteStringValue) | ||
| .setIndex(keyIndex) | ||
| .build() | ||
| ) | ||
| } catch (e: Exception) { | ||
| return CompletableFuture.completedFuture(FlowAccessApi.AccessApiCallResponse.Error("Failed to get account key at latest block", e)) | ||
| } | ||
| ).handle { response, ex -> | ||
| if (ex != null) { | ||
| FlowAccessApi.AccessApiCallResponse.Error("Failed to get account key at latest block", ex) | ||
| } else { | ||
| FlowAccessApi.AccessApiCallResponse.Success(FlowAccountKey.of(response.accountKey)) | ||
| } | ||
| } | ||
| } catch (e: Exception) { | ||
| CompletableFuture.completedFuture(FlowAccessApi.AccessApiCallResponse.Error("Failed to get account key at latest block", e)) | ||
| } | ||
| } | ||
|
|
||
| override fun getAccountKeyAtBlockHeight(address: FlowAddress, keyIndex: Int, height: Long): CompletableFuture<FlowAccessApi.AccessApiCallResponse<FlowAccountKey>> { | ||
| return try { | ||
| completableFuture( | ||
| try { | ||
| api.getAccountKeyAtBlockHeight( | ||
| Access.GetAccountKeyAtBlockHeightRequest | ||
| .newBuilder() | ||
| .setAddress(address.byteStringValue) | ||
| .setIndex(keyIndex) | ||
| .setBlockHeight(height) | ||
| .build() | ||
| ) | ||
| } catch (e: Exception) { | ||
| return CompletableFuture.completedFuture(FlowAccessApi.AccessApiCallResponse.Error("Failed to get account key at block height", e)) | ||
| } | ||
| ).handle { response, ex -> | ||
| if (ex != null) { | ||
| FlowAccessApi.AccessApiCallResponse.Error("Failed to get account key at block height", ex) | ||
| } else { | ||
| FlowAccessApi.AccessApiCallResponse.Success(FlowAccountKey.of(response.accountKey)) | ||
| } | ||
| } | ||
| } catch (e: Exception) { | ||
| CompletableFuture.completedFuture(FlowAccessApi.AccessApiCallResponse.Error("Failed to get account key at block height", e)) | ||
| } | ||
| } | ||
|
|
||
| override fun getAccountKeysAtLatestBlock(address: FlowAddress): CompletableFuture<FlowAccessApi.AccessApiCallResponse<List<FlowAccountKey>>> { | ||
| return try { | ||
| completableFuture( | ||
| try { | ||
| api.getAccountKeysAtLatestBlock( | ||
| Access.GetAccountKeysAtLatestBlockRequest | ||
| .newBuilder() | ||
| .setAddress(address.byteStringValue) | ||
| .build() | ||
| ) | ||
| } catch (e: Exception) { | ||
| return CompletableFuture.completedFuture(FlowAccessApi.AccessApiCallResponse.Error("Failed to get account keys at latest block", e)) | ||
| } | ||
| ).handle { response, ex -> | ||
| if (ex != null) { | ||
| FlowAccessApi.AccessApiCallResponse.Error("Failed to get account keys at latest block", ex) | ||
| } else { | ||
| FlowAccessApi.AccessApiCallResponse.Success(response.accountKeysList.map { FlowAccountKey.of(it) }) | ||
| } | ||
| } | ||
| } catch (e: Exception) { | ||
| CompletableFuture.completedFuture(FlowAccessApi.AccessApiCallResponse.Error("Failed to get account keys at latest block", e)) | ||
| } | ||
| } | ||
|
|
||
| override fun getAccountKeysAtBlockHeight(address: FlowAddress, height: Long): CompletableFuture<FlowAccessApi.AccessApiCallResponse<List<FlowAccountKey>>> { | ||
| return try { | ||
| completableFuture( | ||
| try { | ||
| api.getAccountKeysAtBlockHeight( | ||
| Access.GetAccountKeysAtBlockHeightRequest | ||
| .newBuilder() | ||
| .setAddress(address.byteStringValue) | ||
| .setBlockHeight(height) | ||
| .build() | ||
| ) | ||
| } catch (e: Exception) { | ||
| return CompletableFuture.completedFuture(FlowAccessApi.AccessApiCallResponse.Error("Failed to get account keys at block height", e)) | ||
| } | ||
| ).handle { response, ex -> | ||
| if (ex != null) { | ||
| FlowAccessApi.AccessApiCallResponse.Error("Failed to get account keys at block height", ex) | ||
| } else { | ||
| FlowAccessApi.AccessApiCallResponse.Success(response.accountKeysList.map { FlowAccountKey.of(it) }) | ||
| } | ||
| } | ||
| } catch (e: Exception) { | ||
| CompletableFuture.completedFuture(FlowAccessApi.AccessApiCallResponse.Error("Failed to get account keys at block height", e)) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring common error handling patterns.
The new methods follow consistent patterns but contain duplicated error handling code. Consider extracting common error handling logic into reusable functions.
Here's a suggested approach to reduce duplication:
private fun <T> handleApiCall(
operation: String,
apiCall: () -> ListenableFuture<T>,
responseMapper: (T) -> FlowAccessApi.AccessApiCallResponse<*>
): CompletableFuture<FlowAccessApi.AccessApiCallResponse<*>> {
return try {
completableFuture(
try {
apiCall()
} catch (e: Exception) {
return CompletableFuture.completedFuture(
FlowAccessApi.AccessApiCallResponse.Error("Failed to $operation", e)
)
}
).handle { response, ex ->
if (ex != null) {
FlowAccessApi.AccessApiCallResponse.Error("Failed to $operation", ex)
} else {
responseMapper(response)
}
}
} catch (e: Exception) {
CompletableFuture.completedFuture(
FlowAccessApi.AccessApiCallResponse.Error("Failed to $operation", e)
)
}
}This would allow simplifying the methods to:
override fun getAccountKeyAtLatestBlock(
address: FlowAddress,
keyIndex: Int
): CompletableFuture<FlowAccessApi.AccessApiCallResponse<FlowAccountKey>> {
return handleApiCall(
"get account key at latest block",
{
api.getAccountKeyAtLatestBlock(
Access.GetAccountKeyAtLatestBlockRequest
.newBuilder()
.setAddress(address.byteStringValue)
.setIndex(keyIndex)
.build()
)
}
) { response ->
FlowAccessApi.AccessApiCallResponse.Success(FlowAccountKey.of(response.accountKey))
}
}
There was a problem hiding this comment.
@lealobanov should we consider this suggestion?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@franklywatson Agree, the test files for the Access API endpoints are getting quite large too... I will have a look today to see where I can reduce duplication
There was a problem hiding this comment.
@franklywatson I have just pushed initial refactoring work for the AsyncFlowAccessAPI unit tests - will create a separate issue for refactoring of the remaining files as the size of the changes is quite large
.../src/main/java/org/onflow/examples/java/getAccountKeys/GetAccountKeysAccessAPIConnector.java
Show resolved
Hide resolved
.../src/main/java/org/onflow/examples/java/getAccountKeys/GetAccountKeysAccessAPIConnector.java
Show resolved
Hide resolved
.../src/main/java/org/onflow/examples/java/getAccountKeys/GetAccountKeysAccessAPIConnector.java
Show resolved
Hide resolved
Get tx result by index endpoint
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (14)
kotlin-example/src/main/kotlin/org/onflow/examples/kotlin/getTransaction/GetTransactionAccessAPIConnector.kt (1)
20-24: The implementation looks good but could benefit from documentation and validation.
While the implementation follows the class's established pattern correctly, consider these improvements:
- Add KDoc documentation explaining the method's purpose and parameters
- Add validation for the
indexparameter to ensure it's non-negative
Consider adding documentation and validation:
+ /**
+ * Retrieves a transaction result at the specified index within a block
+ * @param blockId The ID of the block to query
+ * @param index The index of the transaction in the block (must be non-negative)
+ * @return The transaction result
+ * @throws IllegalArgumentException if index is negative
+ * @throws Exception if the API call fails
+ */
fun getTransactionResultByIndex(blockId: FlowId, index: Int): FlowTransactionResult =
+ require(index >= 0) { "Transaction index must be non-negative" }
when (val response = accessAPI.getTransactionResultByIndex(blockId, index)) {
is FlowAccessApi.AccessApiCallResponse.Success -> response.data
is FlowAccessApi.AccessApiCallResponse.Error -> throw Exception(response.message, response.throwable)
}java-example/src/main/java/org/onflow/examples/java/getTransaction/GetTransactionAccessAPIConnector.java (1)
35-36: Consider adding parameter validation.
The method could benefit from parameter validation to ensure robustness:
- Validate that blockId is not null
- Validate that index is not null and non-negative
Consider adding these checks:
public FlowTransactionResult getTransactionResultByIndex(FlowId blockId, Integer index) {
+ if (blockId == null) {
+ throw new IllegalArgumentException("blockId cannot be null");
+ }
+ if (index == null) {
+ throw new IllegalArgumentException("index cannot be null");
+ }
+ if (index < 0) {
+ throw new IllegalArgumentException("index cannot be negative");
+ }
FlowAccessApi.AccessApiCallResponse<FlowTransactionResult> response = accessAPI.getTransactionResultByIndex(blockId, index);Also consider using primitive int instead of Integer for the index parameter since it cannot be null.
kotlin-example/src/test/kotlin/org/onflow/examples/kotlin/getTransaction/GetTransactionAccessAPIConnectorTest.kt (1)
40-43: Enhance error handling in block retrieval.
While the error handling works, it could be more robust and informative:
Consider this improvement:
block = when (val response = accessAPI.getLatestBlock()) {
is FlowAccessApi.AccessApiCallResponse.Success -> response.data
- is FlowAccessApi.AccessApiCallResponse.Error -> throw Exception(response.message, response.throwable)
+ is FlowAccessApi.AccessApiCallResponse.Error -> throw FlowTestException(
+ "Failed to fetch latest block: ${response.message}",
+ response.throwable
+ )
}Add this custom exception class at the top level:
class FlowTestException(message: String, cause: Throwable? = null) : Exception(message, cause)java-example/src/test/java/org/onflow/examples/java/getTransaction/GetTransactionAccessAPIConnectorTest.java (1)
24-24: Consider using a more descriptive field name.
The field name block could be more specific to indicate it represents the latest block, e.g., latestBlock.
- private FlowBlock block;
+ private FlowBlock latestBlock;sdk/src/main/kotlin/org/onflow/flow/sdk/AsyncFlowAccessApi.kt (1)
Line range hint 9-42: Overall implementation looks good with some suggestions for improvement.
The new methods are well-structured and follow the existing patterns in the API. Consider the following architectural improvements:
- Add comprehensive error handling documentation, especially for network-related failures
- Consider adding convenience methods that combine multiple calls (e.g., getting all keys for multiple accounts in one call)
- Consider adding rate limiting or pagination support for methods that might return large datasets
sdk/src/main/kotlin/org/onflow/flow/sdk/FlowAccessApi.kt (1)
21-28: LGTM! Consider adding KDoc documentation.
The new account key methods are well-designed and consistent with the existing API patterns. They follow the established naming conventions and parameter structures.
Consider adding KDoc documentation for these methods to describe:
- Parameter constraints (e.g., valid keyIndex ranges)
- Possible error conditions
- Example usage
kotlin-example/README.md (1)
Line range hint 1-84: Missing documentation for new account key endpoints.
The PR introduces several new account key endpoints (GetAccountKeyAtLatestBlock, GetAccountKeysAtLatestBlock, GetAccountKeyAtBlockHeight, GetAccountKeysAtBlockHeight), but there's no documentation for these new features in the README.
Consider adding a new section under "Get Accounts" to document these new endpoints:
#### Get Accounts
[Get accounts by address.](src/main/kotlin/org/onflow/examples/kotlin/getAccount/GetAccountAccessAPIConnector.kt)
- Get account balance
- Get account from the latest block
- Get account from block by height
+ - Get account key at latest block
+ - Get account keys at latest block
+ - Get account key at block height
+ - Get account keys at block height🧰 Tools
🪛 LanguageTool
[grammar] ~82-~82: This phrase is duplicated. You should probably use “Get transaction” only once.
Context: ...GetTransactionAccessAPIConnector.kt) - Get transaction - Get transaction result - Get transaction result by inde...
(PHRASE_REPETITION)
sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt (1)
58-74: Enhance test coverage with additional assertions.
While the basic test structure is good, consider adding these assertions to make the tests more robust:
- Verify the key's public key format and validity
- Check other key properties (weight, revoked status, etc.)
- For multiple keys, verify they are properly ordered by sequence number
Example for single key test:
assertThat(accountKey).isNotNull
assertThat(accountKey.sequenceNumber).isEqualTo(keyIndex)
+assertThat(accountKey.publicKey).isNotEmpty()
+assertThat(accountKey.weight).isGreaterThan(0)
+assertThat(accountKey.revoked).isFalse()Example for multiple keys test:
assertThat(accountKeys).isNotNull
assertThat(accountKeys).isNotEmpty
+assertThat(accountKeys).isSortedAccordingTo { a, b ->
+ a.sequenceNumber.compareTo(b.sequenceNumber)
+}
+assertThat(accountKeys).allMatch { it.publicKey.isNotEmpty() }Also applies to: 103-118
sdk/src/test/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImplTest.kt (4)
54-65: Consider using more meaningful test data values.
While the mock transaction response is functionally correct, using more descriptive test data would improve test readability and debugging.
- .setBlockId(ByteString.copyFromUtf8("id"))
- .setTransactionId(ByteString.copyFromUtf8("id"))
- .setCollectionId(ByteString.copyFromUtf8("id"))
+ .setBlockId(ByteString.copyFromUtf8("mock_block_id"))
+ .setTransactionId(ByteString.copyFromUtf8("mock_transaction_id"))
+ .setCollectionId(ByteString.copyFromUtf8("mock_collection_id"))170-193: Consider adding edge cases for account keys list.
While the basic functionality is well-tested, consider adding test cases for:
- Empty account keys list
- List with maximum allowed keys
- Keys with different weight values
@Test
fun `Test getAccountKeysAtLatestBlock with empty keys list`() {
val flowAddress = FlowAddress("01")
val response = Access.AccountKeysResponse.newBuilder().build()
`when`(mockApi.getAccountKeysAtLatestBlock(any())).thenReturn(response)
val result = flowAccessApiImpl.getAccountKeysAtLatestBlock(flowAddress)
assertResultSuccess(result) { assertTrue(it.isEmpty()) }
}472-484: Enhance result validation in getTransactionResultByIndex test.
Consider adding more specific assertions to validate individual fields of the transaction result:
val result = flowAccessApiImpl.getTransactionResultByIndex(flowId, index)
- assertResultSuccess(result) { assertEquals(flowTransactionResult, it) }
+ assertResultSuccess(result) {
+ assertEquals(flowTransactionResult.status, it.status)
+ assertEquals(flowTransactionResult.statusCode, it.statusCode)
+ assertEquals(flowTransactionResult.errorMessage, it.errorMessage)
+ assertEquals(flowTransactionResult.blockId, it.blockId)
+ assertEquals(flowTransactionResult.blockHeight, it.blockHeight)
+ }Line range hint 89-499: Consider adding KDoc documentation for test methods.
While the test method names are descriptive, adding KDoc documentation would improve maintainability by clearly stating:
- The purpose of each test
- The expected behavior
- Any specific test conditions or assumptions
Example:
/**
* Verifies that account keys can be successfully retrieved at the latest block.
* Expected behavior:
* 1. API call should return the mocked account key
* 2. Request should be built with correct address and index
*/
@Test
fun `Test getAccountKeyAtLatestBlock`() {
// ... existing test code ...
}sdk/src/test/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImplTest.kt (2)
Line range hint 476-484: Ensure Comprehensive Assertions in Transaction Result Tests
In the getTransactionResultByIndex tests, only limited fields of the FlowTransactionResult are being asserted. For thorough validation, consider asserting all relevant fields to ensure the object is correctly constructed and returned.
Also applies to: 486-515
91-107: Consistent Error Message Verification
Verify that the error messages asserted in failure tests match the actual messages produced by the implementation. This ensures that tests remain reliable even if error messages change due to implementation updates.
Also applies to: 109-122, 124-141, 143-157
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
- java-example/README.md (1 hunks)
- java-example/src/main/java/org/onflow/examples/java/getTransaction/GetTransactionAccessAPIConnector.java (1 hunks)
- java-example/src/test/java/org/onflow/examples/java/getTransaction/GetTransactionAccessAPIConnectorTest.java (4 hunks)
- kotlin-example/README.md (1 hunks)
- kotlin-example/src/main/kotlin/org/onflow/examples/kotlin/getTransaction/GetTransactionAccessAPIConnector.kt (1 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/AsyncFlowAccessApi.kt (2 hunks)
- sdk/src/main/kotlin/org/onflow/flow/sdk/FlowAccessApi.kt (3 hunks)
- sdk/src/main/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImpl.kt (2 hunks)
- sdk/src/main/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImpl.kt (2 hunks)
- sdk/src/test/kotlin/org/onflow/flow/sdk/FlowAccessApiTest.kt (2 hunks)
- sdk/src/test/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImplTest.kt (5 hunks)
- sdk/src/test/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImplTest.kt (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- sdk/src/main/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImpl.kt
🔇 Additional comments (18)
kotlin-example/src/main/kotlin/org/onflow/examples/kotlin/getTransaction/GetTransactionAccessAPIConnector.kt (1)
Line range hint 1-24: Verify the relevance of this change to PR objectives.
This file introduces transaction-related functionality, but the PR objectives specifically mention account key endpoints. Please clarify if this change is intentionally included in this PR or if it should be moved to a separate PR focused on transaction functionality.
Let's check if there are any related changes or dependencies:
java-example/src/main/java/org/onflow/examples/java/getTransaction/GetTransactionAccessAPIConnector.java (2)
35-43: LGTM! Implementation follows established patterns.
The method implementation is consistent with the class's existing error handling patterns and correctly integrates with the FlowAccessAPI.
34-43: Verify the relevance of transaction result changes to PR objectives.
The PR objectives focus on adding account key endpoints, but this file introduces transaction result functionality. Could you clarify how these changes relate to the account key endpoints implementation described in the PR objectives?
Let's check for related changes:
kotlin-example/src/test/kotlin/org/onflow/examples/kotlin/getTransaction/GetTransactionAccessAPIConnectorTest.kt (2)
23-23: LGTM!
The property declaration follows Kotlin conventions and is appropriately scoped.
62-68: Improve test coverage and reliability.
The current test could be more comprehensive and reliable:
- It only tests index 0 and assumes a transaction exists
- It doesn't verify the actual transaction data
- Missing negative test cases
Consider adding:
- A check that the block actually contains transactions
- Multiple test cases with different indices
- Negative test cases for invalid indices
- Verification of transaction details
@Test
fun `Can fetch a transaction result by index`() {
+ // Verify block has transactions
+ assertTrue(block.collectionGuarantees.isNotEmpty(), "Block should contain transactions")
+
val transactionResult = connector.getTransactionResultByIndex(block.id, 0)
assertNotNull(transactionResult, "Transaction result should not be null")
assertTrue(transactionResult.status === FlowTransactionStatus.SEALED, "Transaction should be sealed")
+ // Verify transaction details
+ assertNotNull(transactionResult.blockId, "Block ID should not be null")
+ assertEquals(block.id, transactionResult.blockId, "Block ID should match")
}
+
+@Test
+fun `Should handle invalid transaction index`() {
+ assertThrows<IllegalArgumentException> {
+ connector.getTransactionResultByIndex(block.id, -1)
+ }
+}Let me verify if the block consistently contains transactions:
java-example/src/test/java/org/onflow/examples/java/getTransaction/GetTransactionAccessAPIConnectorTest.java (1)
10-10: LGTM! Import consolidation is acceptable in test files.
sdk/src/main/kotlin/org/onflow/flow/sdk/AsyncFlowAccessApi.kt (2)
41-42: Review return type nullability and add documentation.
The method getTransactionResultByIndex returns a non-nullable FlowTransactionResult while similar methods like getTransactionResultById return nullable FlowTransactionResult?. Please:
- Document why this method guarantees a non-null result
- Consider making the return type consistent with other similar methods
- Add KDoc documentation explaining the relationship between blockId and index parameters
#!/bin/bash
# Check for similar transaction result methods and their return types
rg "getTransaction.*Result.*:.*Flow.*Result" -A 19-16: Add KDoc documentation for the new account key methods.
The new methods would benefit from KDoc documentation describing:
- Parameter constraints (e.g., valid ranges for keyIndex)
- Return value details
- Potential error conditions
- Usage examples
Example format:
/**
* Retrieves an account key at the latest sealed block.
*
* @param address The Flow address to query
* @param keyIndex The index of the key to retrieve (must be non-negative)
* @return A future containing the account key if found
* @throws IllegalArgumentException if keyIndex is negative
*/Consider adding parameter validation hints.
The keyIndex parameter should likely be non-negative. Consider using @IntRange annotation or documenting the valid range.
sdk/src/main/kotlin/org/onflow/flow/sdk/FlowAccessApi.kt (1)
53-54: Add documentation for the index parameter.
While the method signature is consistent with the API design, it would be helpful to document:
- Valid ranges for the index parameter
- How the index corresponds to transaction positions within a block
- Relationship with getTransactionsByBlockId method
Let's check if there are any existing usages that might provide context:
sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt (1)
76-101: Past review comments are still applicable.
The existing review comments about enhancing block height tests with historical validation remain valid and should be implemented.
Also applies to: 120-144
sdk/src/test/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImplTest.kt (1)
89-111: LGTM! Comprehensive test coverage for getAccountKeyAtLatestBlock.
The test properly verifies the successful retrieval of an account key, including request building and response handling.
sdk/src/test/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImplTest.kt (1)
63-73: Validate Initialization of mockTransactionResultResponse
The mockTransactionResultResponse is initialized with default or placeholder values. Ensure that all necessary fields are appropriately set to reflect realistic transaction results for accurate testing.
sdk/src/main/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImpl.kt (6)
46-70: LGTM
The implementation of getAccountKeyAtLatestBlock is correct and follows existing patterns.
72-96: LGTM
The implementation of getAccountKeyAtBlockHeight is correct and consistent with the existing codebase.
99-121: LGTM
The getAccountKeysAtLatestBlock method is implemented correctly and aligns with current coding practices.
124-147: LGTM
The getAccountKeysAtBlockHeight method is correctly implemented and follows the established patterns.
479-503: LGTM
The implementation of getTransactionResultByIndex is correct and adheres to existing conventions.
46-148: Consider refactoring to reduce code duplication
As previously noted, there is duplicated error handling code in these methods. Extracting common logic into reusable functions could enhance maintainability.
Also applies to: 479-503
.../test/java/org/onflow/examples/java/getTransaction/GetTransactionAccessAPIConnectorTest.java
Show resolved
Hide resolved
.../test/java/org/onflow/examples/java/getTransaction/GetTransactionAccessAPIConnectorTest.java
Show resolved
Hide resolved
sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt
Outdated
Show resolved
Hide resolved
| @Test | ||
| fun `Test getAccountKeyAtLatestBlock`() { | ||
| val flowAccessApi = mock(FlowAccessApi::class.java) | ||
| val address = FlowAddress("01") | ||
| val keyIndex = 0 | ||
| val accountKey = FlowAccountKey.of(AccountOuterClass.AccountKey.getDefaultInstance()) | ||
|
|
||
| `when`(flowAccessApi.getAccountKeyAtLatestBlock(address, keyIndex)).thenReturn(FlowAccessApi.AccessApiCallResponse.Success(accountKey)) | ||
|
|
||
| val result = flowAccessApi.getAccountKeyAtLatestBlock(address, keyIndex) | ||
|
|
||
| assertEquals(FlowAccessApi.AccessApiCallResponse.Success(accountKey), result) | ||
| verify(flowAccessApi).getAccountKeyAtLatestBlock(address, keyIndex) | ||
| } | ||
|
|
||
| @Test | ||
| fun `Test getAccountKeyAtBlockHeight`() { | ||
| val flowAccessApi = mock(FlowAccessApi::class.java) | ||
| val address = FlowAddress("01") | ||
| val keyIndex = 0 | ||
| val height = 123L | ||
| val accountKey = FlowAccountKey.of(AccountOuterClass.AccountKey.getDefaultInstance()) | ||
|
|
||
| `when`(flowAccessApi.getAccountKeyAtBlockHeight(address, keyIndex, height)).thenReturn(FlowAccessApi.AccessApiCallResponse.Success(accountKey)) | ||
|
|
||
| val result = flowAccessApi.getAccountKeyAtBlockHeight(address, keyIndex, height) | ||
|
|
||
| assertEquals(FlowAccessApi.AccessApiCallResponse.Success(accountKey), result) | ||
| verify(flowAccessApi).getAccountKeyAtBlockHeight(address, keyIndex, height) | ||
| } | ||
|
|
||
| @Test | ||
| fun `Test getAccountKeysAtLatestBlock`() { | ||
| val flowAccessApi = mock(FlowAccessApi::class.java) | ||
| val address = FlowAddress("01") | ||
| val accountKeys = listOf( | ||
| FlowAccountKey.of(AccountOuterClass.AccountKey.getDefaultInstance()), | ||
| FlowAccountKey.of(AccountOuterClass.AccountKey.getDefaultInstance()) | ||
| ) | ||
|
|
||
| `when`(flowAccessApi.getAccountKeysAtLatestBlock(address)).thenReturn(FlowAccessApi.AccessApiCallResponse.Success(accountKeys)) | ||
|
|
||
| val result = flowAccessApi.getAccountKeysAtLatestBlock(address) | ||
|
|
||
| assertEquals(FlowAccessApi.AccessApiCallResponse.Success(accountKeys), result) | ||
| verify(flowAccessApi).getAccountKeysAtLatestBlock(address) | ||
| } | ||
|
|
||
| @Test | ||
| fun `Test getAccountKeysAtBlockHeight`() { | ||
| val flowAccessApi = mock(FlowAccessApi::class.java) | ||
| val address = FlowAddress("01") | ||
| val height = 123L | ||
| val accountKeys = listOf( | ||
| FlowAccountKey.of(AccountOuterClass.AccountKey.getDefaultInstance()), | ||
| FlowAccountKey.of(AccountOuterClass.AccountKey.getDefaultInstance()) | ||
| ) | ||
|
|
||
| `when`(flowAccessApi.getAccountKeysAtBlockHeight(address, height)).thenReturn(FlowAccessApi.AccessApiCallResponse.Success(accountKeys)) | ||
|
|
||
| val result = flowAccessApi.getAccountKeysAtBlockHeight(address, height) | ||
|
|
||
| assertEquals(FlowAccessApi.AccessApiCallResponse.Success(accountKeys), result) | ||
| verify(flowAccessApi).getAccountKeysAtBlockHeight(address, height) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for account key methods.
The account key tests should be enhanced to align with the existing test patterns in the file:
- Add error scenario tests similar to
getAccountBalanceAtLatestBlock failure - Use meaningful test data instead of default instances
- Verify response properties in detail
Here's an example implementation for getAccountKeyAtLatestBlock:
@Test
fun `Test getAccountKeyAtLatestBlock`() {
val flowAccessApi = mock(FlowAccessApi::class.java)
val address = FlowAddress("01")
val keyIndex = 0
- val accountKey = FlowAccountKey.of(AccountOuterClass.AccountKey.getDefaultInstance())
+ val accountKey = FlowAccountKey(
+ publicKey = ByteString.copyFromUtf8("test-public-key"),
+ signAlgo = SignatureAlgorithm.ECDSA_P256,
+ hashAlgo = HashAlgorithm.SHA2_256,
+ weight = 1000,
+ sequenceNumber = 42,
+ revoked = false
+ )
`when`(flowAccessApi.getAccountKeyAtLatestBlock(address, keyIndex))
.thenReturn(FlowAccessApi.AccessApiCallResponse.Success(accountKey))
val result = flowAccessApi.getAccountKeyAtLatestBlock(address, keyIndex)
assertEquals(FlowAccessApi.AccessApiCallResponse.Success(accountKey), result)
+ assertEquals("test-public-key", result.data.publicKey.toStringUtf8())
+ assertEquals(SignatureAlgorithm.ECDSA_P256, result.data.signAlgo)
+ assertEquals(HashAlgorithm.SHA2_256, result.data.hashAlgo)
+ assertEquals(1000, result.data.weight)
+ assertEquals(42, result.data.sequenceNumber)
+ assertEquals(false, result.data.revoked)
verify(flowAccessApi).getAccountKeyAtLatestBlock(address, keyIndex)
}
+@Test
+fun `Test getAccountKeyAtLatestBlock failure`() {
+ val flowAccessApi = mock(FlowAccessApi::class.java)
+ val address = FlowAddress("01")
+ val keyIndex = 0
+ val exception = RuntimeException("Test exception")
+ val response = FlowAccessApi.AccessApiCallResponse.Error(
+ "Failed to get account key at latest block",
+ exception
+ )
+
+ `when`(flowAccessApi.getAccountKeyAtLatestBlock(address, keyIndex))
+ .thenReturn(response)
+
+ val result = flowAccessApi.getAccountKeyAtLatestBlock(address, keyIndex)
+
+ assertEquals(response, result)
+ verify(flowAccessApi).getAccountKeyAtLatestBlock(address, keyIndex)
+}Committable suggestion was skipped due to low confidence.
| @Test | ||
| fun `Test getTransactionResultByIndex`() { | ||
| val flowAccessApi = mock(FlowAccessApi::class.java) | ||
| val flowId = FlowId("01") | ||
| val index = 0 | ||
|
|
||
| val flowTransactionResult = FlowTransactionResult.of(Access.TransactionResultResponse.getDefaultInstance()) | ||
| `when`(flowAccessApi.getTransactionResultByIndex(flowId, index)).thenReturn(FlowAccessApi.AccessApiCallResponse.Success(flowTransactionResult)) | ||
|
|
||
| val result = flowAccessApi.getTransactionResultByIndex(flowId, index) | ||
|
|
||
| assertEquals(FlowAccessApi.AccessApiCallResponse.Success(flowTransactionResult), result) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for transaction result retrieval.
The transaction result test should be enhanced to align with the existing test patterns in the file:
- Add error scenario test
- Use meaningful test data
- Verify response properties in detail
Here's an example implementation:
@Test
fun `Test getTransactionResultByIndex`() {
val flowAccessApi = mock(FlowAccessApi::class.java)
val flowId = FlowId("01")
val index = 0
- val flowTransactionResult = FlowTransactionResult.of(Access.TransactionResultResponse.getDefaultInstance())
+ val flowTransactionResult = FlowTransactionResult(
+ status = FlowTransactionStatus.SEALED,
+ statusCode = 0,
+ errorMessage = "",
+ events = listOf(
+ FlowEvent(
+ type = "TestEvent",
+ transactionId = flowId,
+ transactionIndex = index,
+ eventIndex = 0,
+ payload = ByteString.copyFromUtf8("test-payload")
+ )
+ ),
+ blockId = flowId,
+ blockHeight = 42L,
+ transactionId = flowId,
+ collectionId = flowId,
+ index = index
+ )
`when`(flowAccessApi.getTransactionResultByIndex(flowId, index))
.thenReturn(FlowAccessApi.AccessApiCallResponse.Success(flowTransactionResult))
val result = flowAccessApi.getTransactionResultByIndex(flowId, index)
assertEquals(FlowAccessApi.AccessApiCallResponse.Success(flowTransactionResult), result)
+ assertEquals(FlowTransactionStatus.SEALED, result.data.status)
+ assertEquals(0, result.data.statusCode)
+ assertEquals("", result.data.errorMessage)
+ assertEquals(1, result.data.events.size)
+ assertEquals("TestEvent", result.data.events[0].type)
+ assertEquals("test-payload", result.data.events[0].payload.toStringUtf8())
+ assertEquals(42L, result.data.blockHeight)
+ assertEquals(index, result.data.index)
}
+@Test
+fun `Test getTransactionResultByIndex failure`() {
+ val flowAccessApi = mock(FlowAccessApi::class.java)
+ val flowId = FlowId("01")
+ val index = 0
+ val exception = RuntimeException("Test exception")
+ val response = FlowAccessApi.AccessApiCallResponse.Error(
+ "Failed to get transaction result by index",
+ exception
+ )
+
+ `when`(flowAccessApi.getTransactionResultByIndex(flowId, index))
+ .thenReturn(response)
+
+ val result = flowAccessApi.getTransactionResultByIndex(flowId, index)
+
+ assertEquals(response, result)
+ verify(flowAccessApi).getTransactionResultByIndex(flowId, index)
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Test | |
| fun `Test getTransactionResultByIndex`() { | |
| val flowAccessApi = mock(FlowAccessApi::class.java) | |
| val flowId = FlowId("01") | |
| val index = 0 | |
| val flowTransactionResult = FlowTransactionResult.of(Access.TransactionResultResponse.getDefaultInstance()) | |
| `when`(flowAccessApi.getTransactionResultByIndex(flowId, index)).thenReturn(FlowAccessApi.AccessApiCallResponse.Success(flowTransactionResult)) | |
| val result = flowAccessApi.getTransactionResultByIndex(flowId, index) | |
| assertEquals(FlowAccessApi.AccessApiCallResponse.Success(flowTransactionResult), result) | |
| } | |
| @Test | |
| fun `Test getTransactionResultByIndex`() { | |
| val flowAccessApi = mock(FlowAccessApi::class.java) | |
| val flowId = FlowId("01") | |
| val index = 0 | |
| val flowTransactionResult = FlowTransactionResult( | |
| status = FlowTransactionStatus.SEALED, | |
| statusCode = 0, | |
| errorMessage = "", | |
| events = listOf( | |
| FlowEvent( | |
| type = "TestEvent", | |
| transactionId = flowId, | |
| transactionIndex = index, | |
| eventIndex = 0, | |
| payload = ByteString.copyFromUtf8("test-payload") | |
| ) | |
| ), | |
| blockId = flowId, | |
| blockHeight = 42L, | |
| transactionId = flowId, | |
| collectionId = flowId, | |
| index = index | |
| ) | |
| `when`(flowAccessApi.getTransactionResultByIndex(flowId, index)) | |
| .thenReturn(FlowAccessApi.AccessApiCallResponse.Success(flowTransactionResult)) | |
| val result = flowAccessApi.getTransactionResultByIndex(flowId, index) | |
| assertEquals(FlowAccessApi.AccessApiCallResponse.Success(flowTransactionResult), result) | |
| assertEquals(FlowTransactionStatus.SEALED, result.data.status) | |
| assertEquals(0, result.data.statusCode) | |
| assertEquals("", result.data.errorMessage) | |
| assertEquals(1, result.data.events.size) | |
| assertEquals("TestEvent", result.data.events[0].type) | |
| assertEquals("test-payload", result.data.events[0].payload.toStringUtf8()) | |
| assertEquals(42L, result.data.blockHeight) | |
| assertEquals(index, result.data.index) | |
| } | |
| @Test | |
| fun `Test getTransactionResultByIndex failure`() { | |
| val flowAccessApi = mock(FlowAccessApi::class.java) | |
| val flowId = FlowId("01") | |
| val index = 0 | |
| val exception = RuntimeException("Test exception") | |
| val response = FlowAccessApi.AccessApiCallResponse.Error( | |
| "Failed to get transaction result by index", | |
| exception | |
| ) | |
| `when`(flowAccessApi.getTransactionResultByIndex(flowId, index)) | |
| .thenReturn(response) | |
| val result = flowAccessApi.getTransactionResultByIndex(flowId, index) | |
| assertEquals(response, result) | |
| verify(flowAccessApi).getTransactionResultByIndex(flowId, index) | |
| } |
sdk/src/test/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImplTest.kt
Outdated
Show resolved
Hide resolved
sdk/src/test/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImplTest.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- java-example/README.md (2 hunks)
- kotlin-example/README.md (2 hunks)
- sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- java-example/README.md
- kotlin-example/README.md
🔇 Additional comments (3)
sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt (3)
76-101: Implement historical validation as suggested in past reviews.
The tests for block height and latest block should be combined to verify consistency across blocks. This was previously suggested in past reviews and is still valid.
Also applies to: 103-118, 120-144
210-216: LGTM! Test name change reflects its purpose.
The test appropriately validates event parsing with specific assertions for event type and content.
58-74: 🛠️ Refactor suggestion
Enhance key validation and extract common test utilities.
The test could be improved by:
- Validating more key properties (e.g., publicKey, weight, etc.)
- Extracting common error handling logic
Consider this enhanced implementation:
private fun getAccountKeyAtLatestBlock(address: FlowAddress, keyIndex: Int = DEFAULT_KEY_INDEX): FlowAccountKey {
return handleResult(
accessAPI.getAccountKeyAtLatestBlock(address, keyIndex),
"Failed to get account key at latest block"
) ?: fail("Account key should not be null")
}
@Test
fun `Can get account key at latest block`() {
val address = serviceAccount.flowAddress
val accountKey = getAccountKeyAtLatestBlock(address)
assertThat(accountKey).isNotNull
assertThat(accountKey.sequenceNumber).isEqualTo(DEFAULT_KEY_INDEX)
assertThat(accountKey.publicKey).isNotNull
assertThat(accountKey.weight).isGreaterThan(0)
}⛔ Skipped due to learnings
Learnt from: franklywatson
PR: onflow/flow-jvm-sdk#55
File: src/intTest/org/onflow/flow/sdk/ExposeAccountKeyIssueTest.kt:151-152
Timestamp: 2024-07-04T05:15:46.310Z
Learning: When reviewing test code, avoid recommending additional exception handling if it might obscure the actual failure points and over-sensitize the process.
Learnt from: franklywatson
PR: onflow/flow-jvm-sdk#55
File: src/intTest/org/onflow/flow/sdk/ExposeAccountKeyIssueTest.kt:151-152
Timestamp: 2024-10-08T17:04:37.869Z
Learning: When reviewing test code, avoid recommending additional exception handling if it might obscure the actual failure points and over-sensitize the process.
sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (1)
sdk/src/test/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImplTest.kt (1)
227-230: Consolidate duplicate code in test setupsThe declarations of
flowAddress,keyIndex, andblockHeightare repeated in multiple tests. Consider extracting these into class-level constants or setup methods to adhere to the DRY (Don't Repeat Yourself) principle.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
sdk/src/test/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImplTest.kt(13 hunks)
🔇 Additional comments (12)
sdk/src/test/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImplTest.kt (12)
11-11: Import eq is correctly added
The addition of import org.mockito.ArgumentMatchers.eq is appropriate and necessary for precise argument matching in your mock setups.
17-17: Import AccountOuterClass is appropriately included
The import of org.onflow.protobuf.entities.AccountOuterClass is required for creating mock account keys and is correctly added.
28-29: Initialization of mock API and AsyncFlowAccessApiImpl
The instantiation of api and asyncFlowAccessApi using Mockito mocks sets up the testing environment effectively.
63-75: Creation of mockBlock for block-related tests
The mockBlock object is properly constructed with relevant fields, facilitating comprehensive testing of block-related functionalities.
77-78: Declaration of flowId and blockId
The initialization of flowId and blockId provides clear identifiers for use in various test cases.
80-85: Setup of mock account keys and test exception
Mocking FlowAccountKey instances and defining a testException ensures that both success and failure scenarios are adequately tested.
94-156: Introduction of MockResponseFactory for reusable mock responses
The MockResponseFactory centralizes the creation of mock responses, reducing code duplication and enhancing maintainability of the test suite.
160-170: Helper functions enhance test clarity
The helper methods like createMockEventsResponse, createMockExecutionResult, and createMockTransaction improve readability and simplify test setups.
188-199: Assertion helpers streamline test validations
The assertSuccess and assertFailure functions provide a consistent approach to validating test outcomes, improving code clarity.
290-333: Exception handling in block-related tests
The block retrieval tests correctly handle potential exceptions and validate both success and failure scenarios, enhancing test coverage.
611-616: Effective testing of execution result retrieval
The getExecutionResultByBlockId test accurately verifies the functionality, ensuring correctness in execution result handling.
634-655: Helper method createMockNodeVersionInfo is well-constructed
The method effectively creates a mock NodeVersionInfo response, facilitating thorough testing of version information retrieval.
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
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
Show resolved
Hide resolved
sdk/src/test/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImplTest.kt
Outdated
Show resolved
Hide resolved
sdk/src/test/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImplTest.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
sdk/src/test/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImplTest.kt (1)
67-67: Use fixed timestamp in tests to ensure consistent resultsUsing
LocalDateTime.now()in tests can lead to non-deterministic behavior, making tests flaky. Consider using a fixed timestamp for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
sdk/src/test/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImplTest.kt(13 hunks)
🧰 Additional context used
📓 Learnings (1)
sdk/src/test/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImplTest.kt (2)
Learnt from: franklywatson
PR: onflow/flow-jvm-sdk#55
File: src/intTest/org/onflow/flow/sdk/ExposeAccountKeyIssueTest.kt:151-152
Timestamp: 2024-07-04T05:15:46.310Z
Learning: When reviewing test code, avoid recommending additional exception handling if it might obscure the actual failure points and over-sensitize the process.
Learnt from: franklywatson
PR: onflow/flow-jvm-sdk#55
File: src/intTest/org/onflow/flow/sdk/ExposeAccountKeyIssueTest.kt:151-152
Timestamp: 2024-10-08T17:04:37.869Z
Learning: When reviewing test code, avoid recommending additional exception handling if it might obscure the actual failure points and over-sensitize the process.
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
Show resolved
Hide resolved
Closes: #126
Description
Add
GetAccountKeyAtLatestBlock, GetAccountKeysAtLatestBlock, GetAccountKeyAtBlockHeight and GetAccountKeysAtBlockHeightmethods to FlowAccessAPIAdd above 4 methods to AsyncFlowAccessAPI
Add new unit tests for above 4 methods
Add new integration tests for above 4 methods
Add examples for
getAccountKeysUpdate examples docs with new methods
Refactor
AsyncFlowAccessAPIunit tests to remove code duplication where possibleFor contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit