#106/ TransactionResult type differs from protobuf schema #116
Conversation
WalkthroughThe changes in this pull request involve updates to documentation and code related to retrieving execution data in the Java and Kotlin examples, as well as modifications to the 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 Results35 files 35 suites 6m 8s ⏱️ Results for commit b1796ae. ♻️ 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: 2
🧹 Outside diff range and nitpick comments (11)
sdk/src/test/kotlin/org/onflow/flow/sdk/models/FlowTransactionResultTest.kt (1)
45-57: Approve changes and suggest improvements for test coverageThe updates to the
FlowTransactionResultconstructor align with the PR objective of matching the protobuf schema. However, there are opportunities to enhance the test coverage:
- Consider using different
FlowIdinstances for each parameter to ensure proper handling of distinct IDs.- Vary the
Longvalues to test different scenarios.- Add comments explaining the purpose of each new parameter to improve code readability.
Here's a suggested improvement:
// Create distinct FlowId instances val flowId1 = FlowId("0x1") val flowId2 = FlowId("0x2") val flowId3 = FlowId("0x3") val flowTransactionResult = FlowTransactionResult( status, invalidStatusCode, errorMessage, emptyList(), flowId1, // blockId: Identifier of the block 1234567890L, // blockHeight: Height of the block flowId2, // collectionId: Identifier of the collection flowId3, // transactionId: Identifier of the transaction 9876543210L // executionEffortCost: Cost of execution effort )This change improves test coverage by using distinct
FlowIdinstances and differentLongvalues, while also adding comments to explain each parameter's purpose.java-example/README.md (2)
69-72: LGTM: New "Get Execution Data" section addedThe new section for "Get Execution Data" is a valuable addition to the README, providing users with information on how to retrieve execution data by block ID. The format and structure are consistent with other sections.
Consider rewording the beginning of the sentence to avoid repetition:
- #### Get Execution Data - - [Get execution data by block ID.] + #### Get Execution Data + + [Retrieve execution data by block ID.]This minor change improves readability by avoiding the repetition of "Get" at the beginning of consecutive sentences.
🧰 Tools
🪛 LanguageTool
[style] ~69-~69: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ollectionAccessAPIConnector.java) #### Get Execution Data [Get execution data by ...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
Line range hint
132-135: Consider updating the "Unsupported Features" sectionWith the addition of the "Get Execution Data" example, it might be worth revisiting the "Unsupported Features" section. While it correctly states that streaming events and execution data are not supported, the new example demonstrates that some form of execution data retrieval is now possible.
Consider updating this section to clarify the distinction between streaming execution data (which is still unsupported) and the newly added capability to retrieve execution data by block ID. This will provide a more accurate representation of the current feature set.
kotlin-example/README.md (5)
14-14: LGTM! Consider adjusting indentation.The addition of "Get execution data" to the table of contents is appropriate and aligns with the new content. It improves the documentation by providing a clear reference to the new section.
To maintain consistent formatting, consider adjusting the indentation:
- - [Get execution data](#get-execution-data) + - [Get execution data](#get-execution-data)🧰 Tools
🪛 Markdownlint
14-14: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
24-24: LGTM! Consider adjusting indentation.The addition of "Streaming events and execution data" to the table of contents is appropriate and aligns with the new content. It improves the documentation by providing a clear reference to the new section.
To maintain consistent formatting, consider adjusting the indentation:
- - [Streaming events and execution data](#streaming-events-and-execution-data) + - [Streaming events and execution data](#streaming-events-and-execution-data)🧰 Tools
🪛 Markdownlint
24-24: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
70-73: LGTM! Consider rephrasing for improved readability.The addition of the "Get Execution Data" section is valuable and aligns with the PR objectives. It provides a clear reference to the relevant Kotlin code example.
To improve readability and avoid repetitive sentence structure, consider rephrasing:
#### Get Execution Data -[Get execution data by block ID.](src/main/kotlin/org/onflow/examples/kotlin/getExecutionData/GetExecutionDataAccessAPIConnector.kt) +This example demonstrates how to [retrieve execution data by block ID](src/main/kotlin/org/onflow/examples/kotlin/getExecutionData/GetExecutionDataAccessAPIConnector.kt).🧰 Tools
🪛 LanguageTool
[style] ~70-~70: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...tCollectionAccessAPIConnector.kt) #### Get Execution Data [Get execution data by ...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
129-135: LGTM! Consider rephrasing for improved readability and clarity.The addition of the "Streaming Events and Execution Data" section is valuable and aligns with the PR objectives. It provides a clear reference to the relevant Kotlin code examples and outlines the main functionalities.
To improve readability and clarity, consider the following changes:
- Rephrase the introduction to avoid repetition:
-[Utilizing the Access API subscription endpoints to stream event and execution data.](src/main/kotlin/org/onflow/examples/kotlin/streaming) +This section demonstrates [how to use the Access API subscription endpoints for streaming events and execution data](src/main/kotlin/org/onflow/examples/kotlin/streaming).
- Use a more consistent format for the list items:
-- Streaming events. -- Streaming events and reconnecting in the event of a failure. -- Streaming execution data. +- How to stream events +- Implementing reconnection logic for event streaming in case of failures +- Techniques for streaming execution dataThese changes will enhance the readability and maintain a consistent style throughout the document.
🧰 Tools
🪛 LanguageTool
[style] ~135-~135: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...connecting in the event of a failure. - Streaming execution data.(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
Line range hint
1-135: Overall, the changes effectively address the PR objectives and improve the documentation.The additions to the README file successfully document the new functionality related to execution data and streaming events. These changes align well with the existing structure of the document and provide clear references to the relevant Kotlin code examples.
While the changes don't directly address the discrepancy between the TransactionResult type and the protobuf schema mentioned in the PR objectives, they do enhance the SDK's documentation, which is a valuable improvement.
To fully meet the PR objectives, consider:
- Adding a note or section specifically addressing the TransactionResult type and its alignment with the protobuf schema.
- Including a brief explanation of how these new features (execution data retrieval and event streaming) relate to the TransactionResult type, if applicable.
🧰 Tools
🪛 LanguageTool
[style] ~70-~70: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...tCollectionAccessAPIConnector.kt) #### Get Execution Data [Get execution data by ...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~74-~74: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ecutionDataAccessAPIConnector.kt) #### Get Network Parameters [Get the current ne...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
sdk/src/test/kotlin/org/onflow/flow/sdk/FlowAccessApiTest.kt (2)
304-326: Approve expanded construction of transaction resultsThe expanded construction of
transactionResult1andtransactionResult2provides more detailed and specific test data. This change aligns with the PR objective of addressing discrepancies between the TransactionResult type and the protobuf schema.Consider adding comments to explain the significance of the new parameters (flowId, 1L, flowId, flowId, 1) to improve test readability and maintainability. For example:
val transactionResult1 = FlowTransactionResult( FlowTransactionStatus.SEALED, 1, "message1", emptyList(), flowId, // Transaction ID 1L, // Block height flowId, // Collection ID flowId, // Block ID 1 // Execution effort )
330-332: Approve updated method calls and suggest method renamingThe method calls to
getTransactionResultsByBlockIdhave been correctly updated to use the renamedflowIdvariable, maintaining consistency with the earlier variable renaming.Consider renaming the
getTransactionResultsByBlockIdmethod togetTransactionResultsByFlowIdfor better consistency with the use offlowId. This would involve changes in theFlowAccessApiinterface and its implementations. Here's an example of how the change might look:-fun getTransactionResultsByBlockId(blockId: FlowId): FlowAccessApi.AccessApiCallResponse<List<FlowTransactionResult>> +fun getTransactionResultsByFlowId(flowId: FlowId): FlowAccessApi.AccessApiCallResponse<List<FlowTransactionResult>>If you decide to make this change, ensure to update all occurrences of the method name throughout the codebase.
sdk/src/main/kotlin/org/onflow/flow/sdk/models.kt (1)
270-275: Add documentation for new propertiesConsider adding KDoc comments for the new properties
blockId,blockHeight,transactionId,collectionId, andcomputationUsagein theFlowTransactionResultdata class. This will improve code readability and help other developers understand the purpose of these fields.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- java-example/README.md (2 hunks)
- kotlin-example/README.md (4 hunks)
- sdk/src/main/kotlin/org/onflow/flow/sdk/models.kt (1 hunks)
- sdk/src/test/kotlin/org/onflow/flow/sdk/FlowAccessApiTest.kt (1 hunks)
- sdk/src/test/kotlin/org/onflow/flow/sdk/models/FlowTransactionResultTest.kt (2 hunks)
🧰 Additional context used
🪛 LanguageTool
java-example/README.md
[style] ~69-~69: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ollectionAccessAPIConnector.java) #### Get Execution Data [Get execution data by ...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
kotlin-example/README.md
[style] ~70-~70: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...tCollectionAccessAPIConnector.kt) #### Get Execution Data [Get execution data by ...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~135-~135: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...connecting in the event of a failure. - Streaming execution data.(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 Markdownlint
kotlin-example/README.md
14-14: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
24-24: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
🔇 Additional comments (6)
sdk/src/test/kotlin/org/onflow/flow/sdk/models/FlowTransactionResultTest.kt (2)
Line range hint
1-101: Summary of review and next stepsThe changes in this test file align well with the PR objective of updating the
TransactionResulttype to match the protobuf schema. TheFlowTransactionResultconstructor now includes additional parameters, which is reflected in both test methods reviewed.To further improve the quality of these changes:
- Enhance test coverage by using distinct
FlowIdinstances and varyingLongvalues in the test cases.- Add comments to explain the purpose of each new parameter in the
FlowTransactionResultconstructor.- Verify that the
getEventsOfTypemethod implementation in theFlowTransactionResultclass has been updated if necessary, and add new test cases if required.As a next step, I recommend reviewing the actual
FlowTransactionResultclass implementation to ensure it fully aligns with these test changes and the protobuf schema. This will help maintain consistency between the implementation and the tests.To assist with this, you can use the following script to locate the
FlowTransactionResultclass definition:#!/bin/bash # Description: Locate the FlowTransactionResult class definition # Test: Search for the FlowTransactionResult class definition rg --type kotlin -A 5 'class FlowTransactionResult' sdk/src/main/kotlinThis will help you quickly locate the class definition for further review and ensure that all changes are consistent across the codebase.
72-83: Approve changes and recommend improvements for comprehensive testingThe updates to the
FlowTransactionResultconstructor in this test method are consistent with the changes in the previous test, which is good for maintaining overall consistency. However, similar improvements can be made here:
- Use distinct
FlowIdinstances for each parameter to ensure proper handling of different IDs.- Vary the
Longvalues to test different scenarios.- Add comments explaining the purpose of each new parameter.
Here's a suggested improvement:
// Create distinct FlowId instances val blockId = FlowId("0x1") val collectionId = FlowId("0x2") val transactionId = FlowId("0x3") val flowTransactionResult = FlowTransactionResult( FlowTransactionStatus.SEALED, 0, "", listOf(event1, event2, event3), blockId, // blockId: Identifier of the block 1234567890L, // blockHeight: Height of the block collectionId, // collectionId: Identifier of the collection transactionId, // transactionId: Identifier of the transaction 9876543210L // executionEffortCost: Cost of execution effort )Additionally, let's verify if the existing test cases still cover all aspects of the
getEventsOfTypemethod with the newFlowTransactionResultstructure:This script will help us confirm if the
getEventsOfTypemethod implementation has been updated to use any of the new fields. If it hasn't, the existing test cases should still be sufficient. However, if it has, we may need to add new test cases to cover the updated functionality.java-example/README.md (2)
14-14: LGTM: Table of contents updated correctlyThe addition of "Get execution data" to the table of contents is consistent with the new section and follows the existing format and order.
Line range hint
1-135: Summary: README updates are appropriate and valuableThe changes to the README file effectively introduce the new "Get Execution Data" functionality. The additions are well-structured, consistent with the existing format, and provide valuable information to users. The updates align well with the PR objectives of addressing issues related to execution data.
A few minor suggestions have been made to further improve clarity and consistency:
- Slight rewording in the new section to avoid word repetition.
- Considering an update to the "Unsupported Features" section to reflect the newly added capability.
Overall, these changes enhance the documentation and provide users with important information about the SDK's capabilities.
🧰 Tools
🪛 LanguageTool
[style] ~69-~69: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ollectionAccessAPIConnector.java) #### Get Execution Data [Get execution data by ...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~73-~73: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...utionDataAccessAPIConnector.java) #### Get Network Parameters [Get the current ne...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
sdk/src/test/kotlin/org/onflow/flow/sdk/FlowAccessApiTest.kt (1)
302-332: Overall assessment: Approve changes with minor suggestionsThe changes in this test case improve clarity, consistency, and alignment with the PR objective of addressing discrepancies between the TransactionResult type and the protobuf schema. The renaming of
blockIdtoflowIdand the expanded construction of transaction results provide more detailed and specific test data.To further enhance the code quality:
- Ensure consistency of the
flowIdusage throughout the test case.- Consider adding comments to explain the significance of the new parameters in the TransactionResult construction.
- Evaluate the suggestion to rename the
getTransactionResultsByBlockIdmethod for better consistency with the use offlowId.These changes contribute positively to the test suite and support the goals outlined in the linked issue #106.
sdk/src/main/kotlin/org/onflow/flow/sdk/models.kt (1)
283-288: Confirm non-nullability of protobuf fieldsWhen mapping the properties
blockId,blockHeight,transactionId,collectionId, andcomputationUsagefrom theAccess.TransactionResultResponse, ensure that these fields are always present and non-null in the protobuf response. If there is a possibility that any of these fields could be null or unset, consider adding null checks or default values to prevent potentialNullPointerExceptions.
| val events: List<FlowEvent>, | ||
| val blockId: FlowId, | ||
| val blockHeight: Long, | ||
| val transactionId: FlowId, | ||
| val collectionId: FlowId, | ||
| val computationUsage: Long |
There was a problem hiding this comment.
Update the builder method to include new properties
The new properties blockId, blockHeight, transactionId, collectionId, and computationUsage have been added to the FlowTransactionResult data class. However, the builder method has not been updated to include these new properties when building the Access.TransactionResultResponse. This omission can lead to inconsistencies when serializing the object back to its protobuf representation.
Apply this diff to update the builder method:
@JvmOverloads
fun builder(builder: Access.TransactionResultResponse.Builder = Access.TransactionResultResponse.newBuilder()): Access.TransactionResultResponse.Builder = builder
.setStatus(TransactionOuterClass.TransactionStatus.valueOf(status.name))
.setStatusCode(statusCode)
.setErrorMessage(errorMessage)
+ .setBlockId(blockId.byteStringValue)
+ .setBlockHeight(blockHeight)
+ .setTransactionId(transactionId.byteStringValue)
+ .setCollectionId(collectionId.byteStringValue)
+ .setComputationUsage(computationUsage)
.addAllEvents(events.map { it.builder().build() })Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
@lealobanov what do you think of this suggestion? Seems maybe reasonable?
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.
Yes will do, thanks!
There was a problem hiding this comment.
This has been updated
Common Integration Test Results1 files 1 suites 7s ⏱️ Results for commit 81f69ee. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
sdk/src/test/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImplTest.kt (1)
165-165: LGTM! Consider adding comments for clarity.The changes to the
FlowTransactionResultconstructor align well with the PR objectives, addressing the discrepancy between the TransactionResult type and the protobuf schema. The addition ofblockId,blockHeight,collectionId,transactionId, andcomputationUsageparameters enhances the object's information content and improves consistency with the protobuf schema.Consider adding inline comments to explain the meaning of each parameter, especially the
Longvalues, to improve readability:val flowTransactionResult = FlowTransactionResult( FlowTransactionStatus.SEALED, 1, // statusCode "message", emptyList(), flowId, // blockId 1L, // blockHeight flowId, // collectionId flowId, // transactionId 1L // computationUsage )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- sdk/src/test/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImplTest.kt (1 hunks)
- sdk/src/test/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImplTest.kt (1 hunks)
sdk/src/test/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImplTest.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Looks good to me, but would request that @jribbink take a look to make sure before merging.
| val events: List<FlowEvent>, | ||
| val blockId: FlowId, | ||
| val blockHeight: Long, | ||
| val transactionId: FlowId, | ||
| val collectionId: FlowId, | ||
| val computationUsage: Long |
There was a problem hiding this comment.
@lealobanov what do you think of this suggestion? Seems maybe reasonable?
There was a problem hiding this comment.
Looks good to me, I would also suggest to make the change @franklywatson suggested before merging
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
sdk/src/test/kotlin/org/onflow/flow/sdk/models/FlowTransactionResultTest.kt (4)
30-34: Add assertions for new fields in the test methodThe test has been updated to include new fields in the
responseBuilder, which is great. However, to ensure complete test coverage, we should also add assertions for these new fields in theflowTransactionResult.Consider adding the following assertions:
assertEquals("blockId", flowTransactionResult.blockId.bytes.toStringUtf8()) assertEquals(1L, flowTransactionResult.blockHeight) assertEquals("transactionId", flowTransactionResult.transactionId.bytes.toStringUtf8()) assertEquals("collectionId", flowTransactionResult.collectionId.bytes.toStringUtf8()) assertEquals(1L, flowTransactionResult.computationUsage)This will verify that the new fields are correctly initialized from the
TransactionResultResponse.
51-63: Consider the relevance of new parameters in this test caseThe
FlowTransactionResultconstructor has been updated with new parameters, which is good for consistency. However, these new parameters (flowId,1L, etc.) are not being used or asserted in this test case.Consider one of the following options:
- If these parameters are relevant to the
throwOnErrorbehavior, add assertions to verify their impact.- If they're not relevant to this specific test, consider creating a helper method or using a test fixture to create a
FlowTransactionResultinstance with default values for these new parameters. This would make the test more focused and easier to read.Example of a helper method:
private fun createTestFlowTransactionResult( status: FlowTransactionStatus, statusCode: Int, errorMessage: String ): FlowTransactionResult { val flowId = FlowId("0x01") return FlowTransactionResult( status, statusCode, errorMessage, emptyList(), flowId, 1L, flowId, flowId, 1L ) }Then you could simplify the test to:
val flowTransactionResult = createTestFlowTransactionResult(status, invalidStatusCode, errorMessage)
78-89: Consider the relevance of new parameters and improve readabilityThe
FlowTransactionResultconstructor has been updated with new parameters, maintaining consistency with the class changes. However, these new parameters are not being used or asserted in this test case.Consider the following suggestions:
- If these parameters are relevant to the
getEventsOfTypebehavior, add assertions to verify their impact.- If they're not relevant to this specific test, consider creating a helper method or using a test fixture to create a
FlowTransactionResultinstance with default values for these new parameters.Additionally, to improve code readability, you could use Kotlin's named arguments feature. For example:
val flowTransactionResult = FlowTransactionResult( status = FlowTransactionStatus.SEALED, statusCode = 0, errorMessage = "", events = listOf(event1, event2, event3), flowId = flowId, someLongValue1 = 1L, flowId2 = flowId, flowId3 = flowId, someLongValue2 = 1L )This makes it clearer what each argument represents, especially for the new parameters.
Line range hint
1-108: Overall good updates, consider improving test coverage and readabilityThe changes in this file successfully accommodate the updates to the
FlowTransactionResultclass, aligning with the PR objectives to address the discrepancy between TransactionResult type and protobuf schema. The test methods have been modified to use the updated constructor, which is good.To further improve the test suite:
- Add assertions for the new fields in the "Test initialization from Transaction Result Response" method to ensure complete coverage.
- Consider creating helper methods or test fixtures for creating
FlowTransactionResultinstances with default values for the new parameters. This would improve readability and focus in test cases where these new fields are not directly relevant.- Use Kotlin's named arguments feature when constructing
FlowTransactionResultinstances to improve code readability.These improvements will enhance the overall quality and maintainability of the test suite.
sdk/src/test/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImplTest.kt (1)
173-184: Enhance readability by defining constants for repeated values.The values
"id"and1Lare used multiple times in this test. For better readability and maintainability, consider defining these values as constants or variables with descriptive names.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- sdk/src/main/kotlin/org/onflow/flow/sdk/models.kt (2 hunks)
- sdk/src/test/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImplTest.kt (1 hunks)
- sdk/src/test/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImplTest.kt (1 hunks)
- sdk/src/test/kotlin/org/onflow/flow/sdk/models/FlowTransactionResultTest.kt (4 hunks)
🧰 Additional context used
🔇 Additional comments (5)
sdk/src/test/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImplTest.kt (1)
165-177: LGTM: Updated test case aligns with new TransactionResult structureThe changes to the
getTransactionResultByIdtest method correctly reflect the updates to theFlowTransactionResultandTransactionResultResponsestructures. The new fields (blockId,blockHeight,transactionId,collectionId, andcomputationUsage) have been added to both the expected result and the mock response, which aligns with the PR objectives of addressing the discrepancy between the TransactionResult type and the protobuf schema.A few observations:
- The new fields are correctly initialized with appropriate test values.
- The order of the fields in the
FlowTransactionResultconstructor matches the order in theTransactionResultResponsebuilder, which is good for readability and maintenance.- The test now covers the new fields, ensuring they are correctly passed through the API.
sdk/src/test/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImplTest.kt (1)
173-184: Updates align with the PR objectives.The changes correctly incorporate the new fields into the
FlowTransactionResultand the corresponding test setup. The code modifications are appropriate and effectively address issue #106.sdk/src/main/kotlin/org/onflow/flow/sdk/models.kt (3)
270-275: New properties added toFlowTransactionResultare correctly defined.The new properties
blockId,blockHeight,transactionId,collectionId, andcomputationUsagehave been appropriately added to theFlowTransactionResultdata class. This enhances the class by providing additional context related to the transaction's execution.
283-288: Companion object'sofmethod correctly initializes new properties.The
ofmethod now initializes the new properties from the corresponding fields inAccess.TransactionResultResponse, ensuring consistency between the protobuf representation and the data class.
297-301:buildermethod updated to include new properties.The
buildermethod now includes the new properties when building theAccess.TransactionResultResponseobject. This addresses the previous feedback and ensures that the serialization back to the protobuf representation includes all necessary fields.
Closes: #106
Description
Adds the missing fields to
FlowTransactionResult:computationUsageAs well as missing fields which the Go SDK had but the JVM SDK did not:
blockIdblockHeightcollectionIdtransactionIdFor contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation