Migrating SDK examples repo into multi-module project structure #62
Conversation
WalkthroughThe changes integrate Java and Kotlin examples into the Changes
Assessment against linked issues
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
lealobanov
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (8)
sdk/README.md (8)
5-6: Clarify SDK installation instructions.The phrase "Use the configuration below to add this SDK to your project using Maven or Gradle." could be clearer by specifying "the Flow JVM SDK".
- Use the configuration below to add this SDK to your project using Maven or Gradle. + Use the configuration below to add the Flow JVM SDK to your project using Maven or Gradle.
10-27: Ensure Maven configuration clarity.The Maven configuration snippet is clear, but ensure that users know to replace
[VERSION HERE]with the actual version.- <version>[VERSION HERE]</version> + <version>[LATEST VERSION]</version>
31-44: Ensure Gradle configuration clarity.The Gradle configuration snippet is clear, but ensure that users know to replace
[VERSION HERE]with the actual version.- api("org.onflow:flow-jvm-sdk:[VERSION HERE]") + api("org.onflow:flow-jvm-sdk:[LATEST VERSION]")
48-67: Ensure Gradle (with test extensions) configuration clarity.The Gradle configuration snippet for test extensions is clear, but ensure that users know to replace
[VERSION HERE]with the actual version.- api("org.onflow:flow-jvm-sdk:[VERSION HERE]") - testFixturesApi(testFixtures("org.onflow:flow-jvm-sdk:[VERSION HERE]")) + api("org.onflow:flow-jvm-sdk:[LATEST VERSION]") + testFixturesApi(testFixtures("org.onflow:flow-jvm-sdk:[LATEST VERSION]"))
73-74: Ensure clarity in example usage instructions.The example usage instructions are clear, but it might be helpful to specify that users should look at the README files within the example modules for detailed instructions.
- Check out the [kotlin-example](../kotlin-example) and [java-example](../java-example) modules in this repository for examples of how to use this SDK in your Kotlin or Java application. + Check out the [kotlin-example](../kotlin-example) and [java-example](../java-example) modules in this repository for examples of how to use this SDK in your Kotlin or Java application. Refer to the README files within these modules for detailed instructions.
78-82: Clarify Flow Emulator requirement.The instructions for tests annotated with
FlowEmulatorTestare clear, but it might be helpful to specify that the Flow Emulator must be running for the tests to work.- Tests annotated with `FlowEmulatorTest` depend on the [Flow Emulator](https://github.com/onflow/flow-emulator), which is part of the [Flow CLI](https://github.com/onflow/flow-cli) to be installed on your machine. + Tests annotated with `FlowEmulatorTest` depend on the [Flow Emulator](https://github.com/onflow/flow-emulator), which is part of the [Flow CLI](https://github.com/onflow/flow-cli) and must be installed and running on your machine.
84-104: Ensure Gradle configuration clarity for integration tests.The Gradle configuration snippet for integration tests is clear, but ensure that users know to replace
[VERSION HERE]with the actual version.- api("org.onflow:flow-jvm-sdk:[VERSION HERE]") - testFixturesApi(testFixtures("org.onflow:flow-jvm-sdk:[VERSION HERE]")) + api("org.onflow:flow-jvm-sdk:[LATEST VERSION]") + testFixturesApi(testFixtures("org.onflow:flow-jvm-sdk:[LATEST VERSION]")
154-155: Avoid split infinitive.Style-wise, it’s not ideal to insert an adverb (‘automatically’) in the middle of an infinitive construction (‘to create’). Try moving the adverb to avoid split infinitives.
- `@FlowTestAccount` - used to automatically create an account in the emulator and inject a `TestAccount` instance containing the new account's credentials. + `@FlowTestAccount` - used to create an account automatically in the emulator and inject a `TestAccount` instance containing the new account's credentials.Tools
LanguageTool
[style] ~154-~154: Style-wise, it’s not ideal to insert an adverb (‘automatically’) in the middle of an infinitive construction (‘to create’). Try moving the adverb to avoid split infinitives.
Context: ...ount credentials -@FlowTestAccount- used to automatically create an account in the emulator and inject a `TestAccou...(SPLIT_INFINITIVE)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- sdk/README.md (1 hunks)
Additional context used
LanguageTool
sdk/README.md
[style] ~154-~154: Style-wise, it’s not ideal to insert an adverb (‘automatically’) in the middle of an infinitive construction (‘to create’). Try moving the adverb to avoid split infinitives.
Context: ...ount credentials -@FlowTestAccount- used to automatically create an account in the emulator and inject a `TestAccou...(SPLIT_INFINITIVE)
Additional comments not posted (1)
sdk/README.md (1)
157-158: Ensure correctness of example test links.Ensure that the links to the example tests are correct and accessible.
Verification successful
Verified: The example test links are correct and accessible.
sdk/src/intTest/org/onflow/flow/sdk/extensions/ProjectTestExtensionsTest.ktsdk/src/intTest/org/onflow/flow/sdk/extensions/TestExtensionsTest.ktScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the example test links are correct and accessible. # Test: Check if the files exist. Expect: Files should exist. fd 'ProjectTestExtensionsTest.kt' | grep 'src/intTest/org/onflow/flow/sdk/extensions/ProjectTestExtensionsTest.kt' fd 'TestExtensionsTest.kt' | grep 'src/intTest/org/onflow/flow/sdk/extensions/TestExtensionsTest.kt'Length of output: 356
flowdb/MANIFEST
Outdated
There was a problem hiding this comment.
We should remove this and add to .gitignore
There was a problem hiding this comment.
@franklywatson , I think the /flowdb is currently persisting the second emulator account which is used to test receipt of the transaction - these tests were failing before because the recipient emulator account was invalid. Let me try to remove from the repo and re-run CICD on Github and let you know. Thanks!
There was a problem hiding this comment.
@franklywatson looks like I got the tests to pass in one go when the Kotlin and Java tests ran against the same emulator instance - so I think should be good now. Also updated .gitignore
Setting up script loader for Cadence scripts
Co-authored-by: j pimmel <[email protected]>
Co-authored-by: j pimmel <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range, codebase verification and nitpick comments (6)
sdk/src/intTest/resources/cadence/test_utils_create_account.cdc (1)
1-29: Add comments to explain each step.Adding comments will improve readability and help others understand the purpose of each step in the script.
+ // Import necessary contracts import FlowToken from 0xFLOWTOKEN import FungibleToken from 0xFUNGIBLETOKEN transaction(startingBalance: UFix64, publicKey: String, signatureAlgorithm: UInt8, hashAlgorithm: UInt8) { prepare(signer: AuthAccount) { + // Create a new account let newAccount = AuthAccount(payer: signer) + // Borrow a reference to the FlowToken vault let provider = signer.borrow<&FlowToken.Vault>(from: /storage/flowTokenVault) ?? panic("Could not borrow FlowToken.Vault reference") + // Borrow a reference to the new account's FungibleToken receiver let newVault = newAccount .getCapability(/public/flowTokenReceiver) .borrow<&{FungibleToken.Receiver}>() ?? panic("Could not borrow FungibleToken.Receiver reference") + // Transfer the starting balance to the new account let coin <- provider.withdraw(amount: startingBalance) newVault.deposit(from: <- coin) + // Add the public key to the new account newAccount.keys.add( publicKey: PublicKey( publicKey: publicKey.decodeHex(), signatureAlgorithm: SignatureAlgorithm(rawValue: signatureAlgorithm)! ), hashAlgorithm: HashAlgorithm(rawValue: hashAlgorithm)!, weight: UFix64(1000) ) } }sdk/src/intTest/org/onflow/flow/sdk/extensions/TestExtensionsTest.kt (3)
91-95: UpdatecodeClasspathLocationpaths to match actual file locations.The specified paths for
ContractInterface.cdcandContractSuccessor.cdcdo not match their actual locations. Please update the paths in the code to the following:
sdk/src/intTest/resources/cadence/test_extensions/ContractInterface.cdc
sdk/src/intTest/resources/cadence/test_extensions/ContractSuccessor.cdcAnalysis chain
Verify the new
codeClasspathLocationpaths.Ensure that the new paths
/cadence/test_extensions/ContractInterface.cdcand/cadence/test_extensions/ContractSuccessor.cdcare correct and the files exist in the specified locations.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the updated contract files. # Test: Check if the files exist in the specified locations. fd 'ContractInterface.cdc' | grep '/cadence/test_extensions/ContractInterface.cdc' fd 'ContractSuccessor.cdc' | grep '/cadence/test_extensions/ContractSuccessor.cdc'Length of output: 305
38-38: IncorrectcodeClasspathLocationpath.The specified path
/cadence/test_extensions/NothingContract.cdcis incorrect. The correct path should besdk/src/intTest/resources/cadence/test_extensions/NothingContract.cdc.
- Incorrect Path:
/cadence/test_extensions/NothingContract.cdc- Correct Path:
sdk/src/intTest/resources/cadence/test_extensions/NothingContract.cdcPlease update the
codeClasspathLocationto reflect the correct path.Analysis chain
Verify the new
codeClasspathLocationpath.Ensure that the new path
/cadence/test_extensions/NothingContract.cdcis correct and the file exists in the specified location.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the updated contract file. # Test: Check if the file exists in the specified location. fd 'NothingContract.cdc' | grep '/cadence/test_extensions/NothingContract.cdc'Length of output: 146
54-54: ThecodeClasspathLocationpath is incorrect.The file
EmptyContract.cdcexists atsdk/src/intTest/resources/cadence/test_extensions/EmptyContract.cdc, but thecodeClasspathLocationis set to/cadence/test_extensions/EmptyContract.cdc. Please update thecodeClasspathLocationto the correct path.
- Incorrect path:
/cadence/test_extensions/EmptyContract.cdc- Correct path:
sdk/src/intTest/resources/cadence/test_extensions/EmptyContract.cdcAnalysis chain
Verify the new
codeClasspathLocationpath.Ensure that the new path
/cadence/test_extensions/EmptyContract.cdcis correct and the file exists in the specified location.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the updated contract file. # Test: Check if the file exists in the specified location. fd 'EmptyContract.cdc' | grep '/cadence/test_extensions/EmptyContract.cdc'Length of output: 140
sdk/src/intTest/org/onflow/flow/sdk/ExposeAccountKeyIssueTest.kt (1)
45-45: Incorrect script path in the code.The specified script path
cadence/expose_account_key_issue/expose_account_key_issue_1.cdcdoes not match the actual pathsdk/src/intTest/resources/cadence/expose_account_key_issue/expose_account_key_issue_1.cdc. Please update the script path in the code to ensure it loads correctly.
- Code:
sdk/src/intTest/org/onflow/flow/sdk/ExposeAccountKeyIssueTest.ktat line 45- Correct Path:
sdk/src/intTest/resources/cadence/expose_account_key_issue/expose_account_key_issue_1.cdcAnalysis chain
Verify the new script path.
Ensure that the new script path
cadence/expose_account_key_issue/expose_account_key_issue_1.cdcis correct and the file exists in the specified location.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the updated script file. # Test: Check if the file exists in the specified location. fd 'expose_account_key_issue_1.cdc' | grep 'cadence/expose_account_key_issue/expose_account_key_issue_1.cdc'Length of output: 196
sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionDecodingTest.kt (1)
57-57: Incorrect script path in code.The script file
transaction_decoding_precompute_txid.cdcis located atsdk/src/intTest/resources/cadence/transaction_decoding/transaction_decoding_precompute_txid.cdcbut the code referencescadence/transaction_decoding/transaction_decoding_precompute_txid.cdc. Update the path in the code to the correct location.
- Incorrect path:
cadence/transaction_decoding/transaction_decoding_precompute_txid.cdc- Correct path:
sdk/src/intTest/resources/cadence/transaction_decoding/transaction_decoding_precompute_txid.cdcAnalysis chain
Verify the new script path.
Ensure that the new script path
cadence/transaction_decoding/transaction_decoding_precompute_txid.cdcis correct and the file exists in the specified location.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the updated script file. # Test: Check if the file exists in the specified location. fd 'transaction_decoding_precompute_txid.cdc' | grep 'cadence/transaction_decoding/transaction_decoding_precompute_txid.cdc'Length of output: 218
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (37)
- NOTICE (1 hunks)
- sdk/src/intTest/org/onflow/flow/sdk/ExposeAccountKeyIssueTest.kt (5 hunks)
- sdk/src/intTest/org/onflow/flow/sdk/IntegrationTestUtils.kt (2 hunks)
- sdk/src/intTest/org/onflow/flow/sdk/cadence/JsonCadenceTest.kt (12 hunks)
- sdk/src/intTest/org/onflow/flow/sdk/extensions/ProjectTestExtensionsTest.kt (2 hunks)
- sdk/src/intTest/org/onflow/flow/sdk/extensions/TestExtensionsTest.kt (4 hunks)
- sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionCreationTest.kt (5 hunks)
- sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionDecodingTest.kt (2 hunks)
- sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt (20 hunks)
- sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionSigningTest.kt (2 hunks)
- sdk/src/intTest/resources/cadence/expose_account_key_issue/expose_account_key_issue_1.cdc (1 hunks)
- sdk/src/intTest/resources/cadence/expose_account_key_issue/expose_account_key_issue_2.cdc (1 hunks)
- sdk/src/intTest/resources/cadence/expose_account_key_issue/expose_account_key_issue_3.cdc (1 hunks)
- sdk/src/intTest/resources/cadence/json_cadence/decode_array.cdc (1 hunks)
- sdk/src/intTest/resources/cadence/json_cadence/decode_boolean.cdc (1 hunks)
- sdk/src/intTest/resources/cadence/json_cadence/decode_complex_dict.cdc (1 hunks)
- sdk/src/intTest/resources/cadence/json_cadence/decode_enum.cdc (1 hunks)
- sdk/src/intTest/resources/cadence/json_cadence/decode_optional.cdc (1 hunks)
- sdk/src/intTest/resources/cadence/json_cadence/decode_optional_2.cdc (1 hunks)
- sdk/src/intTest/resources/cadence/json_cadence/decode_reference.cdc (1 hunks)
- sdk/src/intTest/resources/cadence/json_cadence/decode_resource.cdc (1 hunks)
- sdk/src/intTest/resources/cadence/json_cadence/decode_struct.cdc (1 hunks)
- sdk/src/intTest/resources/cadence/json_cadence/decode_ufix64.cdc (1 hunks)
- sdk/src/intTest/resources/cadence/test_extensions/ContractInterface.cdc (1 hunks)
- sdk/src/intTest/resources/cadence/test_extensions/ContractSuccessor.cdc (1 hunks)
- sdk/src/intTest/resources/cadence/test_extensions/EmptyContract.cdc (1 hunks)
- sdk/src/intTest/resources/cadence/test_utils_create_account.cdc (1 hunks)
- sdk/src/intTest/resources/cadence/transaction_creation/transaction_creation.cdc (1 hunks)
- sdk/src/intTest/resources/cadence/transaction_creation/transaction_creation_simple_transaction.cdc (1 hunks)
- sdk/src/intTest/resources/cadence/transaction_decoding/transaction_decoding_precompute_txid.cdc (1 hunks)
- sdk/src/intTest/resources/cadence/transaction_signing/transaction_signing_byte_arrays.cdc (1 hunks)
- sdk/src/test/kotlin/org/onflow/flow/sdk/ScriptTest.kt (4 hunks)
- sdk/src/test/kotlin/org/onflow/flow/sdk/TestUtils.kt (1 hunks)
- sdk/src/test/resources/cadence/domain_tags.cdc (1 hunks)
- sdk/src/test/resources/cadence/hello_world.cdc (1 hunks)
- sdk/src/test/resources/cadence/import_export_arguments.cdc (1 hunks)
- sdk/src/testFixtures/kotlin/org/onflow/flow/sdk/test/FlowTestUtil.kt (2 hunks)
Files skipped from review due to trivial changes (11)
- sdk/src/intTest/org/onflow/flow/sdk/extensions/ProjectTestExtensionsTest.kt
- sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt
- sdk/src/intTest/resources/cadence/json_cadence/decode_array.cdc
- sdk/src/intTest/resources/cadence/json_cadence/decode_boolean.cdc
- sdk/src/intTest/resources/cadence/json_cadence/decode_optional.cdc
- sdk/src/intTest/resources/cadence/json_cadence/decode_optional_2.cdc
- sdk/src/intTest/resources/cadence/json_cadence/decode_ufix64.cdc
- sdk/src/intTest/resources/cadence/test_extensions/ContractInterface.cdc
- sdk/src/intTest/resources/cadence/test_extensions/EmptyContract.cdc
- sdk/src/intTest/resources/cadence/transaction_decoding/transaction_decoding_precompute_txid.cdc
- sdk/src/test/resources/cadence/hello_world.cdc
Files skipped from review as they are similar to previous changes (1)
- NOTICE
Additional context used
Learnings (3)
sdk/src/intTest/org/onflow/flow/sdk/ExposeAccountKeyIssueTest.kt (1)
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.sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionCreationTest.kt (1)
Learnt from: lealobanov PR: onflow/flow-jvm-sdk#55 File: src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt:95-106 Timestamp: 2024-07-03T12:30:04.496Z Learning: In the `TransactionIntegrationTest` class, exception handling is implemented using `IntegrationTestUtils.handleResult`.sdk/src/test/kotlin/org/onflow/flow/sdk/ScriptTest.kt (1)
Learnt from: franklywatson PR: onflow/flow-jvm-sdk#55 File: src/test/kotlin/org/onflow/flow/sdk/ScriptTest.kt:110-120 Timestamp: 2024-07-04T05:52:07.718Z Learning: When reviewing test code, avoid commenting on exception handling if it's the only issue found.
Additional comments not posted (55)
sdk/src/intTest/resources/cadence/transaction_signing/transaction_signing_byte_arrays.cdc (1)
1-5: Ensure logging does not expose sensitive data.While logging byte arrays might be useful for debugging, ensure that no sensitive information or PII is logged. Consider adding a comment or mechanism to handle sensitive data securely.
sdk/src/intTest/resources/cadence/test_extensions/ContractSuccessor.cdc (1)
1-2: Consider expanding the contract functionality.The contract currently only implements the interface with an empty initializer. Ensure this meets the intended use case, and consider adding more functionality or context if needed.
sdk/src/intTest/resources/cadence/json_cadence/decode_reference.cdc (1)
1-6: LGTM!The code is correct and straightforward. The use of references is appropriate.
sdk/src/intTest/resources/cadence/json_cadence/decode_enum.cdc (1)
1-9: LGTM!The code correctly defines an enum
Colorwith casesred,green, andblue, and a functionmainthat returnsColor.red.sdk/src/intTest/resources/cadence/expose_account_key_issue/expose_account_key_issue_3.cdc (1)
1-5: LGTM!The code correctly defines a transaction that revokes an account key based on the provided index and handles errors using
panic.sdk/src/intTest/resources/cadence/transaction_creation/transaction_creation.cdc (1)
1-6: LGTM!The code correctly defines a transaction that adds a public key to an account.
sdk/src/intTest/resources/cadence/transaction_creation/transaction_creation_simple_transaction.cdc (1)
2-5: LGTM!The transaction script correctly prepares the account and adds the public key.
sdk/src/intTest/resources/cadence/json_cadence/decode_resource.cdc (2)
1-7: LGTM!The resource definition and initializer are correct.
9-12: LGTM!The function correctly creates and returns an instance of the resource.
sdk/src/intTest/resources/cadence/json_cadence/decode_struct.cdc (1)
1-11: LGTM!The struct definition and initializer are correct.
sdk/src/intTest/resources/cadence/expose_account_key_issue/expose_account_key_issue_2.cdc (2)
1-1: Ensure the transaction script parameters are validated.The script assumes that the provided parameters are valid. Consider adding validation checks for the parameters to prevent potential errors or security issues.
9-9: Ensure the weight parameter is within acceptable bounds.The
weightparameter should be validated to ensure it is within the acceptable range (e.g., 0.0 to 1000.0) to prevent potential issues.sdk/src/test/resources/cadence/import_export_arguments.cdc (2)
1-13: LGTM!The
TestClassdefinition is correct and follows best practices.
15-22: LGTM!The
mainfunction correctly initializes and returns an instance ofTestClass.sdk/src/intTest/resources/cadence/json_cadence/decode_complex_dict.cdc (3)
1-13: LGTM! StructStorageInfois correctly defined.The properties are correctly initialized and the types match.
15-21: LGTM! StructFoois correctly defined.The property is correctly initialized and the type matches.
23-31: LGTM! Functionmainis correctly defined.The function correctly retrieves the account, initializes the
Fooinstance, and constructs the dictionary withStorageInfoinstances.sdk/src/test/resources/cadence/domain_tags.cdc (3)
1-2: LGTM! Import statement is correct.The
Cryptomodule is correctly imported.
3-8: LGTM! Function parameters are correctly defined.The parameters are correctly defined and used in the function.
10-40: LGTM! Functionmainis correctly defined.The function correctly creates a
Crypto.KeyList, adds public keys, creates a signature set, and verifies the signatures.sdk/src/intTest/resources/cadence/expose_account_key_issue/expose_account_key_issue_1.cdc (3)
1-3: LGTM! Import statements are correct.The
FlowTokenandFungibleTokenmodules are correctly imported.
4-4: LGTM! Transaction parameters are correctly defined.The parameters are correctly defined and used in the transaction.
5-28: LGTM! Transaction body is correctly defined.The transaction correctly creates a new account, adds a public key, borrows a
FlowToken.Vaultreference, and deposits tokens into the new account.sdk/src/intTest/org/onflow/flow/sdk/IntegrationTestUtils.kt (1)
43-43: Verify return type compatibility.Ensure that
handleResultreturns a compatible type as expected by the callers ofgetAccount.Verification successful
Verified return type compatibility.
The
handleResultfunction correctly returns aFlowAccounttype, which matches the expected return type of thegetAccountfunction.
handleResultreturnsFlowAccountwhen used in the context ofgetAccount.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify return type compatibility of getAccount function. # Test: Search for the function usage. Expect: No type compatibility issues. rg --type kotlin -A 5 $'IntegrationTestUtils.getAccount'Length of output: 1569
Script:
#!/bin/bash # Description: Verify return type compatibility of getAccount function by examining its usage in the identified test files. # Search for the usage of getAccount in ExposeAccountKeyIssueTest.kt rg --type kotlin 'getAccount' sdk/src/intTest/org/onflow/flow/sdk/ExposeAccountKeyIssueTest.kt -A 5 # Search for the usage of getAccount in TransactionCreationTest.kt rg --type kotlin 'getAccount' sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionCreationTest.kt -A 5Length of output: 2142
Script:
#!/bin/bash # Description: Verify the return type of handleResult function. # Search for the implementation of handleResult in the codebase to examine its return type. rg --type kotlin 'fun handleResult' -A 10Length of output: 43
Script:
#!/bin/bash # Description: Locate the implementation of handleResult function to examine its return type. # Search for the definition of handleResult in the codebase. rg 'handleResult' --type kotlin -A 10Length of output: 36196
Script:
#!/bin/bash # Description: Verify the type parameter T used in FlowAccessApi.AccessApiCallResponse to ensure it matches the expected return type of getAccount. # Search for the definition and usage of FlowAccessApi.AccessApiCallResponse in the codebase. rg 'FlowAccessApi.AccessApiCallResponse' --type kotlin -A 10Length of output: 202931
sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionSigningTest.kt (1)
48-51: LGTM! But verify the script path.The changes improve maintainability and readability. Ensure the script path is correct and the file exists.
Verification successful
LGTM! The script path is correct, and the file exists.
The changes improve maintainability and readability. The script path has been verified, and the file
sdk/src/intTest/resources/cadence/transaction_signing/transaction_signing_byte_arrays.cdcexists.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the script path used in the test case. # Test: Check if the script file exists. Expect: The file exists. test -f sdk/src/intTest/resources/cadence/transaction_signing/transaction_signing_byte_arrays.cdc && echo "File exists" || echo "File not found"Length of output: 132
sdk/src/intTest/org/onflow/flow/sdk/extensions/TestExtensionsTest.kt (2)
68-68: Verify the newcodeClasspathLocationpath.Ensure that the new path
/cadence/test_extensions/NothingContract.cdcis correct and the file exists in the specified location.
76-76: Verify the newcodeClasspathLocationpath.Ensure that the new path
/cadence/test_extensions/EmptyContract.cdcis correct and the file exists in the specified location.sdk/src/intTest/org/onflow/flow/sdk/ExposeAccountKeyIssueTest.kt (6)
9-12: LGTM! New imports are necessary and correct.The new import statements for
IntegrationTestUtilsmethods are appropriate and enhance code readability.
14-14: LGTM! New import is necessary and correct.The new import statement for
StandardCharsetsis appropriate and necessary for handling character encoding.
61-64: LGTM! Refactored code is correct and follows best practices.The refactored code for result handling and account address retrieval is appropriate and enhances code readability.
76-76: Verify the new script path.Ensure that the new script path
cadence/expose_account_key_issue/expose_account_key_issue_2.cdcis correct and the file exists in the specified location.
89-89: LGTM! Refactored code is correct and follows best practices.The refactored code for result handling is appropriate and enhances code readability.
99-99: Verify the new script path.Ensure that the new script path
cadence/expose_account_key_issue/expose_account_key_issue_3.cdcis correct and the file exists in the specified location.sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionDecodingTest.kt (2)
7-8: LGTM! New imports are necessary and correct.The new import statements for
IntegrationTestUtilsandStandardCharsetsare appropriate and necessary for handling script loading and character encoding.
60-60: LGTM! Dynamic script loading is correct and follows best practices.The dynamic script loading mechanism enhances maintainability and readability by separating the script logic from the test logic.
sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionCreationTest.kt (6)
11-14: LGTM! Direct imports improve readability.Directly importing utility functions from
IntegrationTestUtilsenhances code readability and reduces namespace dependency.
54-54: LGTM! Simplified function call.Using the direct import of
getAccountsimplifies the code.
64-65: LGTM! Simplified function call.Using the direct import of
loadScriptsimplifies the code.
95-95: LGTM! Simplified function call.Using the direct import of
handleResultsimplifies the code.
97-97: LGTM! Simplified function call.Using the direct import of
handleResultsimplifies the code.
132-132: LGTM! Simplified function call.Using the direct import of
handleResultsimplifies the code.sdk/src/test/kotlin/org/onflow/flow/sdk/ScriptTest.kt (4)
10-10: LGTM! ImportingloadScriptimproves maintainability.Importing the
loadScriptfunction fromTestUtilsallows for dynamic loading of scripts, enhancing maintainability.
55-55: LGTM! Dynamic script loading improves maintainability.Using the
loadScriptfunction to load the script dynamically separates script logic from test code, enhancing maintainability.
78-78: LGTM! Dynamic script loading improves maintainability.Using the
loadScriptfunction to load the script dynamically separates script logic from test code, enhancing maintainability.
139-139: LGTM! Dynamic script loading improves maintainability.Using the
loadScriptfunction to load the script dynamically separates script logic from test code, enhancing maintainability.sdk/src/intTest/org/onflow/flow/sdk/cadence/JsonCadenceTest.kt (8)
5-5: LGTM! ImportingBeforeEachfor setup method.Importing
BeforeEachallows for the setup method to be executed before each test, ensuring consistent test setup.
42-43: LGTM! Usinglateinit varfor consistent availability.Declaring
flowas alateinit varand initializing it in thesetupmethod ensures consistent availability across all test cases without redundant instantiation.
44-47: LGTM! Consistent initialization insetupmethod.The
setupmethod ensures thatflowis consistently initialized before each test, improving test reliability.
49-51: LGTM! Centralized script loading logic.The
loadScriptContentmethod centralizes script loading logic, reducing code duplication and enhancing maintainability.
53-58: LGTM! Abstracted script loading and execution.The
executeScriptmethod abstracts the script loading and execution process, reducing code duplication and improving clarity.
73-73: LGTM! UsingexecuteScriptimproves clarity.The
decodeOptionaltest uses theexecuteScriptmethod to load and execute the script, reducing code duplication and improving clarity.
85-85: LGTM! UsingexecuteScriptimproves clarity.The
decodeOptional2test uses theexecuteScriptmethod to load and execute the script, reducing code duplication and improving clarity.
97-98: LGTM! UsingexecuteScriptimproves clarity.The
decodeBooleantest uses theexecuteScriptmethod to load and execute the script, reducing code duplication and improving clarity.sdk/src/testFixtures/kotlin/org/onflow/flow/sdk/test/FlowTestUtil.kt (2)
14-18: LGTM!The
loadScriptfunction correctly loads a script file from the classpath and handles the case where the resource is not found.
86-93: LGTM!The
createAccountfunction now dynamically loads the script content using theloadScriptfunction, enhancing maintainability.
...c/intTest/resources/cadence/transaction_creation/transaction_creation_simple_transaction.cdc
Show resolved
Hide resolved
| pub fun main(addr: Address): [StorageInfo] { | ||
| let acct = getAccount(addr) | ||
| return [StorageInfo(capacity: 1, used: 2, available: 3)] | ||
| } |
There was a problem hiding this comment.
Ensure proper handling of the account.
The function currently returns a hardcoded StorageInfo instance. Ensure that the function correctly retrieves and returns the storage information for the given account.
pub fun main(addr: Address): [StorageInfo] {
let acct = getAccount(addr)
- return [StorageInfo(capacity: 1, used: 2, available: 3)]
+ let storageInfo = StorageInfo(
+ capacity: acct.storageCapacity,
+ used: acct.storageUsed,
+ available: acct.storageCapacity - acct.storageUsed
+ )
+ return [storageInfo]
}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.
| pub fun main(addr: Address): [StorageInfo] { | |
| let acct = getAccount(addr) | |
| return [StorageInfo(capacity: 1, used: 2, available: 3)] | |
| } | |
| pub fun main(addr: Address): [StorageInfo] { | |
| let acct = getAccount(addr) | |
| let storageInfo = StorageInfo( | |
| capacity: acct.storageCapacity, | |
| used: acct.storageUsed, | |
| available: acct.storageCapacity - acct.storageUsed | |
| ) | |
| return [storageInfo] | |
| } |
There was a problem hiding this comment.
Will revisit content of Cadence files used for tests in subsequent PRs as we will need to ensure Cadence 1.0 compatibility
There was a problem hiding this comment.
@lealobanov, understood. If you need any assistance with ensuring Cadence 1.0 compatibility in future PRs, feel free to reach out. I'll be here to help!
Learnings added
Learnt from: lealobanov
PR: onflow/flow-jvm-sdk#62
File: sdk/src/intTest/resources/cadence/json_cadence/decode_struct.cdc:13-16
Timestamp: 2024-07-19T13:08:57.583Z
Learning: The user plans to revisit the content of Cadence files used for tests in subsequent PRs to ensure Cadence 1.0 compatibility.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
I can take a pass at updating them to 1.0 next week. Will get an SC Eng to review in support
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!
sdk/src/intTest/resources/cadence/expose_account_key_issue/expose_account_key_issue_2.cdc
Show resolved
Hide resolved
sdk/src/intTest/resources/cadence/expose_account_key_issue/expose_account_key_issue_2.cdc
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 5
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- .github/workflows/ci-java-examples-pull-request.yml (1 hunks)
- java-example/build.gradle.kts (1 hunks)
- java-example/src/main/java/org/onflow/examples/java/AccessAPIConnector.java (1 hunks)
- java-example/src/test/java/org/onflow/examples/java/AccessAPIConnectorTest.java (1 hunks)
- kotlin-example/build.gradle.kts (1 hunks)
- kotlin-example/src/main/kotlin/org/onflow/examples/kotlin/AccessAPIConnector.kt (1 hunks)
- kotlin-example/src/test/kotlin/org/onflow/examples/kotlin/AccessAPIConnectorTest.kt (1 hunks)
- sdk/src/intTest/org/onflow/flow/sdk/IntegrationTestUtils.kt (2 hunks)
- sdk/src/test/kotlin/org/onflow/flow/sdk/TestUtils.kt (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ci-java-examples-pull-request.yml
- sdk/src/test/kotlin/org/onflow/flow/sdk/TestUtils.kt
Additional context used
Gitleaks
kotlin-example/src/test/kotlin/org/onflow/examples/kotlin/AccessAPIConnectorTest.kt
12-12: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
java-example/src/test/java/org/onflow/examples/java/AccessAPIConnectorTest.java
14-14: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
Additional comments not posted (43)
java-example/build.gradle.kts (5)
3-13: LGTM!The plugins and helper function are well-structured and follow best practices.
29-34: LGTM!The repository configurations are comprehensive and well-structured.
36-42: LGTM!The dependencies are well-chosen and follow best practices.
44-47: LGTM!The application configuration is appropriate and correctly specifies the main class.
49-56: LGTM!The test and copy task configurations are appropriate and follow best practices.
kotlin-example/build.gradle.kts (5)
3-13: LGTM!The plugins and helper function are well-structured and follow best practices.
29-34: LGTM!The repository configurations are comprehensive and well-structured.
36-42: LGTM!The dependencies are well-chosen and follow best practices.
44-47: LGTM!The application configuration is appropriate and correctly specifies the main class.
49-56: LGTM!The test and copy task configurations are appropriate and follow best practices.
kotlin-example/src/test/kotlin/org/onflow/examples/kotlin/AccessAPIConnectorTest.kt (6)
1-9: LGTM!The package declaration and imports are appropriate for the test cases.
10-14: LGTM!The constants and class declaration are well-defined and appropriate.
Tools
Gitleaks
12-12: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
18-23: LGTM!The setup method is well-structured and follows best practices.
25-31: LGTM!The test method for creating an account is well-structured and follows best practices.
33-46: LGTM!The test method for transferring tokens is well-structured and follows best practices.
48-53: LGTM!The test method for getting an account balance is well-structured and follows best practices.
sdk/src/intTest/org/onflow/flow/sdk/IntegrationTestUtils.kt (2)
6-8: LGTM!The
loadScriptfunction correctly handles null safety by throwing anIllegalArgumentExceptionif the script is not found.
45-45: LGTM!The
getAccountfunction correctly uses thehandleResultfunction to process the result without casting toFlowAccount. The change ensures type safety and robustness.java-example/src/test/java/org/onflow/examples/java/AccessAPIConnectorTest.java (4)
20-24: LGTM!The
setupUserfunction correctly generates a key pair and sets the public key hex for the user.
26-33: LGTM!The
createAccountfunction correctly creates an account and asserts that the account is not null.
35-52: LGTM!The
transferTokensfunction correctly transfers tokens and asserts that the balance is updated correctly.
54-60: LGTM!The
getAccountBalancefunction correctly retrieves the account balance and asserts that the balance is not null.kotlin-example/src/main/kotlin/org/onflow/examples/kotlin/AccessAPIConnector.kt (9)
10-14: LGTM!The constructor and properties are implemented correctly.
18-21: LGTM!The
getAccountBalancefunction correctly retrieves the account balance.
23-26: LGTM!The
getAccountKeyfunction correctly retrieves an account key at the specified index.
28-34: LGTM!The
getTransactionResultfunction correctly retrieves the transaction result and throws an exception if there is an error message.
36-45: LGTM!The
waitForSealfunction correctly waits for a transaction to be sealed.
47-56: LGTM!The
getAccountCreatedAddressfunction correctly retrieves the account address from the transaction result.
58-58: LGTM!The
loadScriptfunction correctly reads a script from the resources using the class loader.
60-101: LGTM!The
createAccountfunction correctly creates an account using the provided payer address and public key hex.
103-138: LGTM!The
transferTokensfunction correctly transfers tokens from the sender address to the recipient address.java-example/src/main/java/org/onflow/examples/java/AccessAPIConnector.java (12)
23-26: Verify error handling forCrypto.decodePrivateKey.Ensure that
Crypto.decodePrivateKeyhandles invalid input gracefully and throws appropriate exceptions.
28-61: Handle missing or invalid scripts gracefully.Ensure that the
loadScriptmethod handles missing or invalid scripts gracefully and throws appropriate exceptions.
28-61: Verify transaction success before returning the created account address.Ensure that the transaction was successful before returning the created account address.
63-91: Verify transaction success before returning.Ensure that the transaction was successful before returning.
63-91: Handle possible exceptions during the transaction.Ensure that possible exceptions during the transaction are handled gracefully.
93-95: LGTM!The method is straightforward and appears to be correct.
97-100: LGTM!The method is straightforward and appears to be correct.
102-104: LGTM!The method is straightforward and appears to be correct.
106-109: LGTM!The method is straightforward and appears to be correct.
111-113: LGTM!The method is straightforward and appears to be correct.
132-147: Handle cases where the transaction result does not contain the expected event.Ensure that the method handles cases where the transaction result does not contain the expected event.
149-157: Handle cases where the script is not found or cannot be read.Ensure that the method handles cases where the script is not found or cannot be read.
| private FlowTransactionResult waitForSeal(FlowId txID) { | ||
| FlowTransactionResult txResult; | ||
|
|
||
| while(true) { | ||
| txResult = this.getTransactionResult(txID); | ||
| if (txResult.getStatus().equals(FlowTransactionStatus.SEALED)) { | ||
| return txResult; | ||
| } | ||
|
|
||
| try { | ||
| Thread.sleep(1000L); | ||
| } catch (InterruptedException e) { | ||
| e.printStackTrace(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Optimize the busy-wait loop.
Consider using a more efficient mechanism to wait for the transaction to be sealed, such as an event-driven approach or a backoff strategy.
| import static org.junit.jupiter.api.Assertions.*; | ||
|
|
||
| class AccessAPIConnectorTest { | ||
| public static final String SERVICE_PRIVATE_KEY_HEX = "a2f983853e61b3e27d94b7bf3d7094dd756aead2a813dd5cf738e1da56fa9c17"; |
There was a problem hiding this comment.
Address the potential issue with the exposed API key.
The SERVICE_PRIVATE_KEY_HEX is flagged as a potential issue. Ensure that this key is securely managed and not exposed in the source code.
Tools
Gitleaks
14-14: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
| private fun getAccount(address: FlowAddress): FlowAccount = accessAPI.getAccountAtLatestBlock(address)!! | ||
|
|
There was a problem hiding this comment.
Consider adding null safety.
The getAccount function uses the non-null assertion operator. Consider adding null safety to handle cases where the account is not found.
- private fun getAccount(address: FlowAddress): FlowAccount = accessAPI.getAccountAtLatestBlock(address)!!
+ private fun getAccount(address: FlowAddress): FlowAccount? = accessAPI.getAccountAtLatestBlock(address)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.
| private fun getAccount(address: FlowAddress): FlowAccount = accessAPI.getAccountAtLatestBlock(address)!! | |
| private fun getAccount(address: FlowAddress): FlowAccount? = accessAPI.getAccountAtLatestBlock(address) |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- sdk/src/intTest/org/onflow/flow/sdk/IntegrationTestUtils.kt (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- sdk/src/intTest/org/onflow/flow/sdk/IntegrationTestUtils.kt
SDK Examples Unit Test Results2 files 2 suites 2s ⏱️ Results for commit 36dfe1e. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- .github/workflows/ci-examples-pull-request.yml (1 hunks)
- .gitignore (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- .gitignore
Additional comments not posted (4)
.github/workflows/ci-examples-pull-request.yml (4)
1-9: LGTM!The workflow metadata and environment setup are appropriate for a Java project.
24-32: LGTM!The steps for setting up Java and installing the Flow emulator are appropriate.
34-50: LGTM!The steps for building the project and starting the emulator are appropriate.
52-69: LGTM!The steps for running unit tests and publishing test results are appropriate.
Description
This PR accomplishes:
Next steps out of this PR:
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
Documentation
README.mdwith a clearer structure and detailed module descriptions for the SDK, Java Example, and Kotlin Example.Chores
NOTICEfile to reflect the current year and attribution.