Investigating issues with ECDSA_SECP256k1

lealobanov commented 6 months ago

Investigating reported GH issues with ECDSA_SECP256k1

For contributor use:

[ ] Targeted PR against master branch
[ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
[ ] Code follows the standards mentioned here.
[ ] Updated relevant documentation
[ ] Re-reviewed Files changed in the Github PR explorer
[ ] Added appropriate labels

Summary by CodeRabbit

New Features
- Introduced logging support using SLF4J and Logback.
- Added SdkConfig to allow custom logger integration.
- Expanded cryptographic capabilities with new algorithms KECCAK256 and KMAC128.
Documentation
- Updated README with instructions for integrating a custom logger.
Tests
- Added extensive tests for logging and new cryptographic functionalities.
Enhancements
- Improved HashAlgorithm to include KECCAK256 and KMAC128.
- Refined cryptographic methods for better performance and flexibility.

coderabbitai[bot] commented 6 months ago

[!IMPORTANT]

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

The updates add custom logging and expand cryptographic functionalities within the SDK for Java and Kotlin applications. This involves integrating SLF4J and Logback, adding new cryptographic algorithms, and including comprehensive tests for these features. Improvements to logging include the introduction of a custom logger through SdkConfig and an example provided in the README.

Changes

File/Directory	Summary
`README.md`	Added section on integrating custom logger
`build.gradle.kts`	Added `slf4j-api` and `logback-classic` dependencies
`src/main/kotlin/.../LoggerProvider.kt`	Introduced `LoggerProvider` singleton for managing loggers
`src/main/kotlin/.../SdkConfig.kt`	Added `SdkConfig` class for setting custom loggers
`src/main/kotlin/.../crypto/Crypto.kt`	Updated cryptographic functions and added parameters
`src/main/kotlin/.../models.kt`	Corrected enum and added `KECCAK256` and `KMAC128` values
`src/main/kotlin/resources/logback.xml`	Introduced Logback configuration
`src/test/kotlin/.../LoggerProviderTest.kt`	Added tests for `LoggerProvider`
`src/test/kotlin/.../SdkConfigTest.kt`	Added tests for `SdkConfig`
`src/test/kotlin/.../crypto/CryptoTest.kt`	Added tests for cryptographic methods
`src/test/kotlin/.../models/HashAlgorithmTest.kt`	Added tests for new hash algorithms
`src/test/kotlin/.../ScriptTest.kt`	Minor changes to struct `TestClass` and `main` function

Poem

In the realm of code, with bytes and logs, Where hashes mix with frogs and dogs. A custom logger sets the scene, With SLF4J, our apps are keen. Crypto enhanced, so safe and sound, In Java's land, new logs abound. 🌟🐇

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?

Share

- [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)

Tips

### Chat There are 3 ways to chat with [CodeRabbit](https://coderabbit.ai): - Review comments: Directly reply to a review comment made by CodeRabbit. Example: - `I pushed a fix in commit .` - `Generate unit testing code for this file.` - `Open a follow-up GitHub issue for this discussion.` - Files and specific lines of code (under the "Files changed" tab): Tag `@coderabbitai` in a new review comment at the desired location with your query. Examples: - `@coderabbitai generate unit testing code for this file.` - `@coderabbitai modularize this function.` - PR comments: Tag `@coderabbitai` in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples: - `@coderabbitai generate interesting stats about this repository and render them as a table.` - `@coderabbitai show all the console.log statements in this repository.` - `@coderabbitai read src/utils.ts and generate unit testing code.` - `@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.` - `@coderabbitai help me debug CodeRabbit configuration file.` 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) - `@coderabbitai pause` to pause the reviews on a PR. - `@coderabbitai resume` to resume the paused reviews. - `@coderabbitai review` to trigger an incremental review. This is useful when automatic reviews are disabled for the repository. - `@coderabbitai full review` to do a full review from scratch and review all the files again. - `@coderabbitai summary` to regenerate the summary of the PR. - `@coderabbitai resolve` resolve all the CodeRabbit review comments. - `@coderabbitai configuration` to show the current CodeRabbit configuration for the repository. - `@coderabbitai help` to get help. Additionally, you can add `@coderabbitai ignore` anywhere in the PR description to prevent this PR from being reviewed. ### CodeRabbit Configration File (`.coderabbit.yaml`) - You can programmatically configure CodeRabbit by adding a `.coderabbit.yaml` file to the root of your repository. - Please see the [configuration documentation](https://docs.coderabbit.ai/guides/configure-coderabbit) for more information. - If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: `# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json` ### Documentation and Community - Visit our [Documentation](https://coderabbit.ai/docs) for detailed information on how to use CodeRabbit. - Join our [Discord Community](https://discord.com/invite/GsXnASn26c) to get help, request features, and share feedback. - Follow us on [X/Twitter](https://twitter.com/coderabbitai) for updates and announcements.

github-actions[bot] commented 6 months ago

Unit Test Results

  53 files ±0   53 suites ±0 7s :stopwatch: ±0s 292 tests +5 291 :heavy_check_mark: +4 0 :zzz: ±0 1 :x: +1

For more details on these failures, see this check.

Results for commit 59feb70c. ± Comparison against base commit 797007a9.

:recycle: This comment has been updated with latest results.

github-actions[bot] commented 6 months ago

Integration Test Results

0 files -   7 0 suites - 7 0s :stopwatch: -27s 0 tests - 33 0 :heavy_check_mark: - 31 0 :zzz: ±0 0 :x: - 2

Results for commit 38f19fb3. ± Comparison against base commit a51dee88.

This pull request removes 33 tests.

``` org.onflow.flow.sdk.cadence.JsonCadenceTest ‑ Can parse new JSON Cadence() org.onflow.flow.sdk.cadence.JsonCadenceTest ‑ decodeArray() org.onflow.flow.sdk.cadence.JsonCadenceTest ‑ decodeBoolean() org.onflow.flow.sdk.cadence.JsonCadenceTest ‑ decodeComplexDict() org.onflow.flow.sdk.cadence.JsonCadenceTest ‑ decodeEnum() org.onflow.flow.sdk.cadence.JsonCadenceTest ‑ decodeOptional() org.onflow.flow.sdk.cadence.JsonCadenceTest ‑ decodeOptional2() org.onflow.flow.sdk.cadence.JsonCadenceTest ‑ decodeReference() org.onflow.flow.sdk.cadence.JsonCadenceTest ‑ decodeResource() org.onflow.flow.sdk.cadence.JsonCadenceTest ‑ decodeStruct() … ```

:recycle: This comment has been updated with latest results.

tarakby commented 5 months ago

I'll also review this PR, but it seems to have changes from https://github.com/onflow/flow-jvm-sdk/pull/52 too, so I'll wait for #52 to be merged to see the specific changes of #53 clearer.

tarakby commented 4 months ago

Thanks for rebasing the changes to the hashing branch 👌🏼 The PR title mentions ECDSA issues with secp256k1, is there a reference to where those issues are described? This is to help with the review. Thanks

lealobanov commented 4 months ago

@tarakby , yes the related issue is: https://github.com/the-nft-company/flow-jvm-sdk/issues/24 which links to 2 more follow-up issues. Will follow up on latest comments for hashing PR tomorrow as well. Thanks!

tarakby commented 4 months ago

I think #71 covered the same issues addressed by this current PR, so it should be fine to close this PR.

onflow / flow-jvm-sdk