sdk/src/test/kotlin/org/onflow/flow/sdk/models/FlowAggregatedSignatureTest.kt (3)

44-66: LGTM: Comprehensive test for of function with a minor suggestion

This test effectively verifies the FlowAggregatedSignature.of function by mocking the protobuf object and checking the conversion result. It covers both the size and content of the resulting lists.

Consider adding a test case with multiple signatures and signer IDs to ensure the function handles lists correctly:

Mockito.`when`(aggregatedSignatureProto.verifierSignaturesList).thenReturn(
    listOf(
        com.google.protobuf.ByteString.copyFrom("signature1".toByteArray()),
        com.google.protobuf.ByteString.copyFrom("signature2".toByteArray())
    )
)
Mockito.`when`(aggregatedSignatureProto.signerIdsList).thenReturn(
    listOf(
        com.google.protobuf.ByteString.copyFrom("signerId1".toByteArray()),
        com.google.protobuf.ByteString.copyFrom("signerId2".toByteArray())
    )
)

val flowAggregatedSignature = FlowAggregatedSignature.of(aggregatedSignatureProto)

assertEquals(2, flowAggregatedSignature.verifierSignatures.size)
assertEquals(2, flowAggregatedSignature.signerIds.size)
// Add assertions for individual elements

---

68-82: LGTM: Good test for builder function with suggestions for improvement

This test effectively verifies the builder() function of the FlowAggregatedSignature class. It checks that the builder produces the correct ByteString representations of the original data.

Consider enhancing the test with the following improvements:

1. Add a more complex case with multiple signatures and signer IDs:

val signature1 = FlowSignature("signature1".toByteArray())
val signature2 = FlowSignature("signature2".toByteArray())
val signerId1 = FlowId.of("signerId1".toByteArray())
val signerId2 = FlowId.of("signerId2".toByteArray())

val aggregatedSignature = FlowAggregatedSignature(
    verifierSignatures = listOf(signature1, signature2),
    signerIds = listOf(signerId1, signerId2)
)

val builderResult = aggregatedSignature.builder()

assertEquals(listOf(signature1.byteStringValue, signature2.byteStringValue), builderResult.verifierSignaturesList)
assertEquals(listOf(signerId1.byteStringValue, signerId2.byteStringValue), builderResult.signerIdsList)

2. Add a test for deep equality by converting the builder result back to a FlowAggregatedSignature:

val reconstructedSignature = FlowAggregatedSignature.of(builderResult.build())
assertEquals(aggregatedSignature, reconstructedSignature)

These additions will provide more comprehensive coverage of the builder() function and ensure that the round-trip conversion works correctly.

---

1-11: Consider adding more test cases and class documentation

The test class provides good coverage of the main functionalities of FlowAggregatedSignature. To further improve the test suite, consider the following suggestions:

1. Add a brief documentation comment for the class explaining its purpose:

/**
 * Unit tests for the FlowAggregatedSignature class.
 * These tests cover equality, hash code, conversion from protobuf, and the builder pattern.
 */
class FlowAggregatedSignatureTest {
    // ... existing code ...
}

2. Add test cases for potential edge cases or error scenarios, such as:

   • Empty lists of signatures or signer IDs
   • Very large lists of signatures or signer IDs
   • Invalid input data (e.g., mismatched sizes of signatures and signer IDs)

3. Consider adding a test for the toString() method of FlowAggregatedSignature if it's implemented.

These additions will enhance the robustness and documentation of your test suite.

sdk/src/test/kotlin/org/onflow/flow/sdk/models/FlowBlockSealTest.kt (4)

13-53: LGTM: Comprehensive equality and hash code test.

This test thoroughly checks the equals and hashCode methods of FlowBlockSeal, including all properties. It's particularly valuable as it covers the newly added properties such as finalState, resultId, and aggregatedApprovalSigs.

Consider adding a negative test case where one property differs between the two instances to ensure the equals method correctly identifies inequality.

---

55-85: LGTM: Thorough test for protobuf conversion.

This test effectively verifies the FlowBlockSeal.of function, ensuring correct conversion from the protobuf representation. It covers all properties, including the newly added ones like resultId and finalState.

Consider adding assertions for executionReceiptSignatures, resultApprovalSignatures, and aggregatedApprovalSigs to ensure complete coverage of all properties during conversion.

---

87-120: LGTM: Comprehensive test for the builder function.

This test effectively verifies the FlowBlockSeal builder functionality, ensuring all properties are correctly reconstructed, including the newly added ones like resultId and finalState.

Consider adding an assertion for aggregatedApprovalSigs to ensure complete coverage of all properties in the builder test.

---

1-120: Great job on comprehensive test coverage for FlowBlockSeal!

This new test file aligns well with the PR objectives, particularly in addressing the addition of missing protobuf fields to the FlowBlockSeal model. The tests cover equality, protobuf conversion, and builder functionality, including checks for newly added properties like finalState, resultId, and aggregatedApprovalSigs.

The test structure is clear, and the use of JUnit and Mockito is appropriate. The tests effectively verify the changes made to the FlowBlockSeal class, ensuring its correctness and compatibility with the updated protobuf schema.

To further enhance the test suite:

1. Consider adding negative test cases for equality checks.
2. Ensure all properties are asserted in the protobuf conversion and builder tests.
3. If not already present elsewhere, consider adding integration tests that use actual protobuf serialization/deserialization to complement these unit tests.

Configuration used: CodeRabbit UI
Review profile: CHILL

sdk/src/test/kotlin/org/onflow/flow/sdk/models/FlowBlockSealTest.kt (3)

55-105: LGTM: Thorough test for protobuf conversion with room for improvement.

This test effectively verifies the FlowBlockSeal.of function, ensuring accurate conversion from a protobuf representation to a FlowBlockSeal instance. It covers all properties, including the newly added ones.

Consider enhancing the test by adding more assertions for list properties:

• Check the size of executionReceiptSignatures, resultApprovalSignatures, and aggregatedApprovalSigs lists.
• Verify the content of these lists more thoroughly, especially for aggregatedApprovalSigs.

Example:

assertEquals(1, flowBlockSeal.executionReceiptSignatures.size)
assertEquals(FlowSignature("signature1".toByteArray()), flowBlockSeal.executionReceiptSignatures[0])
// Similar assertions for other list properties

---

107-140: LGTM: Comprehensive test for builder function with a minor improvement suggestion.

This test effectively verifies the FlowBlockSeal builder function, ensuring all properties are correctly reconstructed. It covers all properties, including the newly added ones.

Consider adding an assertion for the aggregatedApprovalSigs property in the builder result. This would ensure complete coverage of all properties:

assertEquals(1, builderResult.aggregatedApprovalSigsList.size)
val builtAggregatedSig = builderResult.aggregatedApprovalSigsList[0]
assertEquals(aggregatedSignature1.verifierSignatures.map { it.byteStringValue }, builtAggregatedSig.verifierSignaturesList)
assertEquals(aggregatedSignature1.signerIds.map { it.byteStringValue }, builtAggregatedSig.signerIdsList)

---

1-140: Excellent test coverage for FlowBlockSeal with minor enhancement opportunities.

The FlowBlockSealTest class provides comprehensive test coverage for the FlowBlockSeal class, including the newly added properties and functionalities. The tests are well-structured, using appropriate JUnit 5 and Mockito features.

Key strengths:

1. Thorough testing of equality and hash code functionality.
2. Comprehensive verification of protobuf conversion.
3. Detailed testing of the builder function.

These tests align well with the PR objectives, ensuring that the FlowBlockSeal model accurately reflects the expected structure defined in the protobuf schema.

To further enhance the test suite:

1. Consider adding edge cases, such as empty lists or null values where applicable.
2. Implement parameterized tests for different scenarios to increase coverage.
3. Add tests for any public methods of FlowBlockSeal that are not currently covered.

These enhancements will contribute to a more robust test suite, further ensuring the reliability of the FlowBlockSeal class.

Configuration used: CodeRabbit UI
Review profile: CHILL

sdk/src/main/kotlin/org/onflow/flow/sdk/models.kt (1)

1204-1260: LGTM! Consider adding documentation for new properties.

The changes to FlowBlockSeal class look good. The new properties finalState, resultId, and aggregatedApprovalSigs have been added as per the PR objectives. The renaming of id to blockId improves clarity. The equals() and hashCode() methods have been correctly updated to include the new properties.

Consider adding KDoc comments for the new properties to improve code documentation and maintainability.

Configuration used: CodeRabbit UI
Review profile: CHILL

kotlin-example/src/main/kotlin/org/onflow/examples/kotlin/getAccountBalance/GetAccountBalanceAccessAPIConnector.kt (1)

3-3: Consider using specific imports instead of wildcard

While wildcard imports can be convenient, explicit imports make dependencies clearer and prevent potential naming conflicts.

-import org.onflow.flow.sdk.*
+import org.onflow.flow.sdk.FlowAccessApi
+import org.onflow.flow.sdk.FlowAddress

java-example/src/main/java/org/onflow/examples/java/getAccountBalance/GetAccountBalanceAccessAPIConnector.java (1)

6-11: Add class-level documentation and constructor validation.

Consider adding:

1. JavaDoc documentation explaining the class's purpose and usage
2. Null validation in the constructor

Apply these changes:

+/**
+ * Connector class for retrieving account balances from the Flow blockchain using the Flow Access API.
+ * This class provides methods to fetch balances at the latest block or at a specific block height.
+ */
 public class GetAccountBalanceAccessAPIConnector {
     private final FlowAccessApi accessAPI;
 
     public GetAccountBalanceAccessAPIConnector(FlowAccessApi accessAPI) {
+        if (accessAPI == null) {
+            throw new IllegalArgumentException("accessAPI cannot be null");
+        }
         this.accessAPI = accessAPI;
     }

kotlin-example/src/test/kotlin/org/onflow/examples/kotlin/getAccountBalance/GetAccountBalanceAccessAPIConnectorTest.kt (1)

39-39: Extract sealed block flag to a constant.

The boolean parameter true in getLatestBlock calls should be extracted to a named constant for better readability and maintainability.

+    companion object {
+        private const val SEALED_BLOCK_ONLY = true
+    }
+
     @Test
     fun `Can fetch account balance at a specific block height`() {
         val address = serviceAccount.flowAddress
-        val latestBlock = accessAPI.getLatestBlock(true)
+        val latestBlock = accessAPI.getLatestBlock(SEALED_BLOCK_ONLY)

Also applies to: 60-60

java-example/src/test/java/org/onflow/examples/java/getAccountBalance/GetAccountBalanceAccessAPIConnectorTest.java (2)

30-36: Enhance assertion message for better test failure diagnosis.

The assertion message could be more descriptive by including the actual balance value.

-        Assertions.assertTrue(balance >= 0, "Balance at the latest block should be non-negative");
+        Assertions.assertTrue(balance >= 0, 
+            String.format("Balance at the latest block should be non-negative, but got: %d", balance));

---

30-76: Add test coverage for edge cases.

The current test suite could benefit from additional test cases:

1. Test with zero balance accounts
2. Test with invalid block heights
3. Test with maximum block height
4. Test with non-existent accounts

Would you like me to help generate these additional test cases?

sdk/src/main/kotlin/org/onflow/flow/sdk/FlowAccessApi.kt (1)

29-32: Add KDoc documentation for the new balance retrieval methods.

The method signatures look good and follow the interface's patterns. However, consider adding KDoc documentation to clarify:

• The denomination of the returned balance (e.g., FLOW tokens in smallest unit)
• Any constraints on the height parameter
• Error conditions that might trigger an Error response

Example documentation:

/**
 * Retrieves the account balance at the latest sealed block.
 *
 * @param address The Flow address to query
 * @return Success with balance in FLOW smallest denomination, or Error if the account doesn't exist
 */
fun getAccountBalanceAtLatestBlock(address: FlowAddress): AccessApiCallResponse<Long>

/**
 * Retrieves the account balance at a specific block height.
 *
 * @param address The Flow address to query
 * @param height The block height to query (must be <= latest block height)
 * @return Success with balance in FLOW smallest denomination, or Error if the account/block doesn't exist
 */
fun getAccountBalanceAtBlockHeight(address: FlowAddress, height: Long): AccessApiCallResponse<Long>

sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt (2)

144-171: Extract magic number to a named constant.

The normalization factor 100_000_000L appears to be a significant value for Flow token denomination. Consider extracting it to a named constant for better maintainability and documentation.

+ private companion object {
+     // Number of decimal places in Flow token (1 FLOW = 10^8 decimal places)
+     const val FLOW_TOKEN_DECIMALS = 100_000_000L
+ }

     @Test
     fun `Can get account balance at latest block`() {
         // ...
-        val normalizedBalance = balanceResponse / 100_000_000L
+        val normalizedBalance = balanceResponse / FLOW_TOKEN_DECIMALS

---

144-211: Tests are well-structured but could benefit from test data variation.

The tests effectively verify the new account balance retrieval functionality and maintain consistency with the existing test patterns. However, consider enhancing the test suite with:

1. A parameterized test that verifies balance retrieval with different account states and balance values.
2. Edge cases such as accounts with zero balance or maximum possible balance.

This would provide more comprehensive coverage while maintaining the existing clean architecture.

sdk/src/test/kotlin/org/onflow/flow/sdk/FlowAccessApiTest.kt (2)

91-119: Consider enhancing test coverage for getAccountBalanceAtLatestBlock.

While the basic success and failure cases are well-implemented, consider adding the following test cases for better coverage:

1. Edge cases: zero balance, maximum balance
2. Invalid inputs: malformed addresses
3. Consistent error message format validation

Example test case for zero balance:

@Test
fun `Test getAccountBalanceAtLatestBlock with zero balance`() {
    val flowAccessApi = mock(FlowAccessApi::class.java)
    val flowAddress = FlowAddress("0x1234567890abcdef")  // More realistic address
    val expectedBalance = 0L
    val response = FlowAccessApi.AccessApiCallResponse.Success(expectedBalance)

    `when`(flowAccessApi.getAccountBalanceAtLatestBlock(flowAddress)).thenReturn(response)

    val result = flowAccessApi.getAccountBalanceAtLatestBlock(flowAddress)

    assertEquals(response, result)
    verify(flowAccessApi).getAccountBalanceAtLatestBlock(flowAddress)
}

---

121-151: Consider enhancing test coverage for getAccountBalanceAtBlockHeight.

While the basic success and failure cases are well-implemented, consider adding the following test cases for better coverage:

1. Edge cases: zero balance, maximum balance
2. Invalid inputs: negative block height, non-existent block height
3. Consistent error message format validation

Example test case for non-existent block height:

@Test
fun `Test getAccountBalanceAtBlockHeight with non-existent block height`() {
    val flowAccessApi = mock(FlowAccessApi::class.java)
    val flowAddress = FlowAddress("0x1234567890abcdef")  // More realistic address
    val nonExistentHeight = Long.MAX_VALUE
    val exception = RuntimeException("Block height not found")
    val response = FlowAccessApi.AccessApiCallResponse.Error(
        "Failed to get account balance: block height not found",
        exception
    )

    `when`(flowAccessApi.getAccountBalanceAtBlockHeight(flowAddress, nonExistentHeight))
        .thenReturn(response)

    val result = flowAccessApi.getAccountBalanceAtBlockHeight(flowAddress, nonExistentHeight)

    assertEquals(response, result)
    verify(flowAccessApi).getAccountBalanceAtBlockHeight(flowAddress, nonExistentHeight)
}

sdk/src/main/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImpl.kt (2)

99-110: Consider adding parameter validation and documentation.

While the implementation follows the class patterns, consider these improvements:

1. Add null check for the address parameter
2. Add KDoc comments describing:
   • Parameter requirements
   • Possible error cases
   • Balance representation (units, precision)

+/**
+ * Retrieves the account balance at the latest block.
+ *
+ * @param address The Flow address to query (must not be null)
+ * @return Success with the account balance in FLOW units, or Error if the operation fails
+ * @throws IllegalArgumentException if address is null
+ */
 override fun getAccountBalanceAtLatestBlock(address: FlowAddress): FlowAccessApi.AccessApiCallResponse<Long> =
     try {
+        requireNotNull(address) { "Address must not be null" }
         val ret = api.getAccountBalanceAtLatestBlock(

---

112-124: Add parameter validation and documentation for block height method.

While the implementation follows the class patterns, consider these improvements:

1. Add parameter validation:
   • Null check for address
   • Non-negative check for height
2. Add KDoc comments describing:
   • Parameter requirements
   • Possible error cases
   • Balance representation

+/**
+ * Retrieves the account balance at the specified block height.
+ *
+ * @param address The Flow address to query (must not be null)
+ * @param height The block height to query (must be non-negative)
+ * @return Success with the account balance in FLOW units, or Error if the operation fails
+ * @throws IllegalArgumentException if address is null or height is negative
+ */
 override fun getAccountBalanceAtBlockHeight(address: FlowAddress, height: Long): FlowAccessApi.AccessApiCallResponse<Long> =
     try {
+        requireNotNull(address) { "Address must not be null" }
+        require(height >= 0) { "Height must be non-negative" }
         val ret = api.getAccountBalanceAtBlockHeight(

sdk/src/test/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImplTest.kt (5)

208-223: Consider adding edge cases and parameter verification.

While the basic success case is well-tested, consider:

1. Testing edge cases (e.g., zero balance, maximum balance)
2. Verifying that the correct address is passed to the API call using argument captors

// Example of parameter verification
val addressCaptor = ArgumentCaptor.forClass(Access.GetAccountBalanceAtLatestBlockRequest::class.java)
verify(api).getAccountBalanceAtLatestBlock(addressCaptor.capture())
assertEquals(flowAddress.bytes, addressCaptor.value.address.toByteArray())

---

225-237: Enhance error scenario coverage.

Consider adding tests for specific error cases:

• Invalid address format
• Network-related exceptions (e.g., IOException)
• Service-specific exceptions

@Test
fun `test getAccountBalanceAtLatestBlock with invalid address`() {
    val invalidAddress = FlowAddress("invalid")
    val exception = IllegalArgumentException("Invalid address format")
    
    `when`(api.getAccountBalanceAtLatestBlock(any())).thenThrow(exception)
    
    val result = asyncFlowAccessApi.getAccountBalanceAtLatestBlock(invalidAddress).get()
    assert(result is FlowAccessApi.AccessApiCallResponse.Error)
    assertEquals("Invalid address format", (result as FlowAccessApi.AccessApiCallResponse.Error).throwable.message)
}

---

239-255: Add boundary condition tests for block height.

Consider testing:

• Minimum valid block height (0 or 1)
• Very large block heights
• Non-existent block heights

@Test
fun `test getAccountBalanceAtBlockHeight with minimum height`() {
    val flowAddress = FlowAddress("01")
    val minBlockHeight = 0L
    val expectedBalance = 1000L
    val balanceResponse = Access.AccountBalanceResponse
        .newBuilder()
        .setBalance(expectedBalance)
        .build()
    
    `when`(api.getAccountBalanceAtBlockHeight(any())).thenReturn(setupFutureMock(balanceResponse))
    
    val result = asyncFlowAccessApi.getAccountBalanceAtBlockHeight(flowAddress, minBlockHeight).get()
    assert(result is FlowAccessApi.AccessApiCallResponse.Success)
    assertEquals(expectedBalance, (result as FlowAccessApi.AccessApiCallResponse.Success).data)
}

---

257-270: Add block height-specific error cases.

Consider adding tests for:

• Block height not found
• Block height out of range
• Future block heights

@Test
fun `test getAccountBalanceAtBlockHeight with non-existent block`() {
    val flowAddress = FlowAddress("01")
    val nonExistentHeight = Long.MAX_VALUE
    val exception = IllegalArgumentException("Block height not found")
    
    `when`(api.getAccountBalanceAtBlockHeight(any())).thenThrow(exception)
    
    val result = asyncFlowAccessApi.getAccountBalanceAtBlockHeight(flowAddress, nonExistentHeight).get()
    assert(result is FlowAccessApi.AccessApiCallResponse.Error)
    assertEquals("Block height not found", (result as FlowAccessApi.AccessApiCallResponse.Error).throwable.message)
}

---

208-270: Consider implementing a shared test fixture.

To reduce code duplication and improve maintainability, consider:

1. Creating shared test data fixtures for common objects (addresses, balances, etc.)
2. Implementing helper methods for common test scenarios
3. Using a test lifecycle management approach (e.g., @BeforeEach) for setup

private companion object TestData {
    val TEST_ADDRESS = FlowAddress("01")
    val TEST_BALANCE = 1000L
    
    fun createBalanceResponse(balance: Long = TEST_BALANCE) = 
        Access.AccountBalanceResponse.newBuilder()
            .setBalance(balance)
            .build()
}

sdk/src/test/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImplTest.kt (2)

188-200: Consider enhancing error message specificity.

While the error handling test is good, consider making the error message more specific by including the address in the error message for better debugging.

-        assertEquals("Failed to get account balance at latest block", (result as FlowAccessApi.AccessApiCallResponse.Error).message)
+        assertEquals("Failed to get account balance at latest block for address: ${flowAddress.formatted}", (result as FlowAccessApi.AccessApiCallResponse.Error).message)

---

226-239: Consider enhancing error message specificity.

Similar to the latest block failure test, consider making the error message more specific by including both the address and block height for better debugging.

-        assertEquals("Failed to get account balance at block height", (result as FlowAccessApi.AccessApiCallResponse.Error).message)
+        assertEquals("Failed to get account balance at block height ${blockHeight} for address: ${flowAddress.formatted}", (result as FlowAccessApi.AccessApiCallResponse.Error).message)

sdk/src/main/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImpl.kt (1)

180-204: LGTM! Consider standardizing error message format.

The implementation is correct and well-structured. However, for consistency with other methods in the codebase, consider standardizing the error message format.

-                    return CompletableFuture.completedFuture(FlowAccessApi.AccessApiCallResponse.Error("Failed to get account balance at block height", e))
+                    return CompletableFuture.completedFuture(FlowAccessApi.AccessApiCallResponse.Error("Failed to get account balance by block height", e))

This makes it consistent with other methods that use "by" instead of "at" in error messages (e.g., getAccountByBlockHeight).

Configuration used: CodeRabbit UI
Review profile: CHILL