Skip to content

Conversation

@adityashirsatrao007
Copy link
Contributor

Fixes #993. Adds unit tests for keccak256, compress_point, and decompress_point in crypto_utils.py.

Copilot AI review requested due to automatic review settings December 8, 2025 13:49
@github-actions
Copy link

github-actions bot commented Dec 8, 2025

Hi, this is MergeConflictBot.
Your pull request cannot be merged because it contains merge conflicts.

Please resolve these conflicts locally and push the changes.

To assist you, please read:

Thank you for contributing!

From the Hiero Python SDK Team

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds comprehensive unit tests for the crypto_utils module to address issue #993, covering keccak256, compress_point_unchecked, decompress_point, and compress_with_cryptography functions. The PR also includes an enhancement to the Good First Issue template by adding an Acceptance Criteria section.

  • Adds 4 unit test functions covering cryptographic utility functions with known test vectors and round-trip validation
  • Enhances the Good First Issue template with standardized acceptance criteria
  • Updates CHANGELOG formatting (though introduces some issues that need correction)

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
tests/unit/crypto_utils_test.py New comprehensive test suite for crypto_utils module with test cases for keccak256 hashing, point compression/decompression, and validation against known vectors
CHANGELOG.md Formatting updates and whitespace changes (missing entry for #993, contains formatting issues)
.github/ISSUE_TEMPLATE/01_good_first_issue.yml Adds Acceptance Criteria section to improve contributor guidance with clear merge requirements checklist

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@adityashirsatrao007 adityashirsatrao007 force-pushed the test/issue-993-crypto-utils branch from 823d8e7 to 98aa8df Compare December 8, 2025 13:54
Copy link
Contributor

@exploreriii exploreriii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please can you make sure you always create a fork from an updated main, that is in sync with the upstream main
Otherwise you will get odd artefacts on your changelog and etc which hold you back :)

Copy link
Contributor

@exploreriii exploreriii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please follow docs/sdk_developers/rebasing.md, pull all changes from upstream, rebase your branch

and then add a changelog entry please just with your change
Else the tests will fail :(

@exploreriii exploreriii marked this pull request as draft December 8, 2025 14:53
@exploreriii
Copy link
Contributor

Just changed it to draft while you make the tweaks, pelase click ready to review once its ready again :)

@adityashirsatrao007 adityashirsatrao007 force-pushed the test/issue-993-crypto-utils branch from 98aa8df to 27a104f Compare December 8, 2025 15:11
@adityashirsatrao007 adityashirsatrao007 marked this pull request as ready for review December 8, 2025 15:12
@exploreriii
Copy link
Contributor

request review @tech0priyanshu if able

@exploreriii
Copy link
Contributor

Request review @AntonioCeppellini

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@exploreriii
Copy link
Contributor

exploreriii commented Dec 10, 2025

Hi @adityashirsatrao007 what do you think of the copilot feedback?
Please note you also have a merge conflict 👍

edit: marking as draft while you apply changes and mark it ready to review again

@exploreriii exploreriii marked this pull request as draft December 10, 2025 00:02
@adityashirsatrao007 adityashirsatrao007 force-pushed the test/issue-993-crypto-utils branch from 27a104f to 3e36344 Compare December 10, 2025 09:48
@adityashirsatrao007 adityashirsatrao007 marked this pull request as ready for review December 10, 2025 09:49
@adityashirsatrao007 adityashirsatrao007 force-pushed the test/issue-993-crypto-utils branch 2 times, most recently from 0a2de6e to a66e0a6 Compare December 10, 2025 10:22
@adityashirsatrao007
Copy link
Contributor Author

Hi @adityashirsatrao007 what do you think of the copilot feedback? Please note you also have a merge conflict 👍

edit: marking as draft while you apply changes and mark it ready to review again

Done

Copy link
Contributor

@tech0priyanshu tech0priyanshu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @adityashirsatrao007 , for this contribution! 🎉 Please make the update, and then we’ll be ready to go.

@adityashirsatrao007 adityashirsatrao007 force-pushed the test/issue-993-crypto-utils branch from 3b67e65 to 8aa283c Compare December 10, 2025 11:02
@adityashirsatrao007
Copy link
Contributor Author

Implemented Copilot suggestions:

  • Added edge case tests for decompress_point (ValueError checks).
  • Added additional test vector for keccak256.
  • Verified pytest passes locally.

Check updated code.

Copy link
Contributor

@exploreriii exploreriii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @adityashirsatrao007
note when using tools, they are often not signature attached so you can lose sign status if you use them

Please follow instructions here to rectify
https://github.com/hiero-ledger/hiero-sdk-python/pull/1006/checks?check_run_id=57655613240

Else you can do a soft revert and recommit

@exploreriii exploreriii marked this pull request as draft December 10, 2025 11:04
@adityashirsatrao007 adityashirsatrao007 force-pushed the test/issue-993-crypto-utils branch from d1d5713 to 6d5e74e Compare December 10, 2025 11:15
@adityashirsatrao007 adityashirsatrao007 marked this pull request as ready for review December 10, 2025 11:16
Signed-off-by: adityashirsatrao007 <adityashirsatrao007@gmail.com>
Signed-off-by: adityashirsatrao007 <adityashirsatrao007@gmail.com>
Signed-off-by: adityashirsatrao007 <adityashirsatrao007@gmail.com>
Signed-off-by: adityashirsatrao007 <adityashirsatrao007@gmail.com>
Signed-off-by: adityashirsatrao007 <adityashirsatrao007@gmail.com>
@exploreriii exploreriii merged commit aff0f16 into hiero-ledger:main Dec 10, 2025
14 checks passed
@exploreriii
Copy link
Contributor

Thank you, great going @adityashirsatrao007 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Create tests/unit/crypto_utils_test.py

3 participants