Feat/crypto data hash predicate #474
Conversation
|
Note Reviews PausedUse the following commands to manage reviews:
WalkthroughThe update introduces a new predicate Changes
Poem
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 9
Configuration used: CodeRabbit UI
Files selected for processing (10)
- docs/predicate/predicates.md (3 hunks)
- x/logic/interpreter/registry.go (2 hunks)
- x/logic/predicate/crypto.go (6 hunks)
- x/logic/predicate/crypto_test.go (4 hunks)
- x/logic/predicate/util.go (3 hunks)
- x/logic/predicate/util_test.go (3 hunks)
- x/logic/util/crypto.go (2 hunks)
- x/logic/util/crypto_enum.go (1 hunks)
- x/logic/util/prolog.go (3 hunks)
- x/logic/util/prolog_test.go (1 hunks)
Additional comments: 24
x/logic/interpreter/registry.go (1)
- 112-112: The new predicate
crypto_data_hash/3is added to the registry. Ensure that the functionpredicate.CryptoDataHashis correctly implemented and tested.x/logic/util/crypto_enum.go (1)
- 1-73: The code looks good and follows the best practices for defining and handling enumerated types in Go. The
HashAlgtype is well-defined with constants for MD5, SHA-256, and SHA-512 hash algorithms. The functions for obtaining string representations, checking validity, and parsing strings intoHashAlgvalues are correctly implemented. The error handling for invalidHashAlgvalues is also appropriate.x/logic/predicate/util_test.go (3)
102-113: The new test case for UTF-8 encoding is a good addition. It ensures that the new encoding option is working as expected.
126-129: The error message for an invalid encoding option has been improved to include a list of possible values. This is a good practice as it provides more information to the user, helping them to correct their mistake.
168-174: The function signature for
TermToByteshas been updated to include an additional parameter for the encoding type. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.x/logic/util/prolog_test.go (1)
- 33-34: Ensure that the
GetOptionfunction handles the case where theoptionis an atom and theoptionsis a compound term correctly.x/logic/predicate/util.go (1)
- 18-26: The new encoding options are well defined and documented.
x/logic/util/prolog.go (4)
13-19: The introduction of
AtomEmptyListis a good way to represent an empty list. Ensure that this new global variable is used consistently throughout the codebase.58-67: The
IsListfunction has been updated to handleengine.Atomtypes in addition toengine.Compoundtypes. This change seems to be in line with the goal of handling both types.69-75: The
IsEmptyListfunction has been updated to handleengine.Atomtypes. This change seems to be in line with the goal of handling both types.100-111: The
GetOptionfunction now returnsnilif the resolved term is an empty list. This seems to be a logical change, as an empty list would not contain any options.x/logic/predicate/crypto.go (5)
16-22: The
SHAHashfunction is deprecated and replaced byCryptoDataHash. Ensure all calls toSHAHashare updated to useCryptoDataHashinstead.52-121: The new
CryptoDataHashfunction provides more options for hashing algorithms and data encoding. It's a good improvement over the deprecatedSHAHashfunction.211-213: The
EDDSAVerifyfunction now uses autil.KeyAlgtype instead ofutil.Algfor specifying cryptographic algorithms. This change should be reflected in all calls toEDDSAVerify.251-253: The
ECDSAVerifyfunction now uses autil.KeyAlgtype instead ofutil.Algfor specifying cryptographic algorithms. This change should be reflected in all calls toECDSAVerify.307-322: The helper function
getOptionAsAtomWithDefaultis a good addition for handling options and resolving atoms. It improves code readability and maintainability.x/logic/util/crypto.go (5)
1-15: Ensure that all the necessary packages are imported correctly. Also, make sure that the
go-enumpackage is installed and working as expected.31-48: The
HashAlgtype andHashermethod are correctly implemented. However, consider adding a default case in the switch statement to handle unsupported hash algorithms.1-54: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [50-66]
The
VerifySignaturefunction is correctly implemented. However, consider adding a default case in the switch statement to handle unsupported key algorithms.
70-79: The
Hashfunction is correctly implemented. It uses theHashermethod to get a hash function for the given algorithm and then uses it to hash the input data.67-83: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [81-96]
The
verifySignatureWithCurvefunction is correctly implemented. It verifies the ASN1 signature of the given message with the given public key using the given elliptic curve.x/logic/predicate/crypto_test.go (1)
- 193-193: Ensure that all calls to the deprecated
sha_hash/2function throughout the codebase have been replaced with the newcrypto_data_hash/3function.docs/predicate/predicates.md (2)
180-222: The new
crypto_data_hash/3function is well-documented with clear examples and explanations of the parameters and options. Ensure that all references to the deprecatedsha_hash/2function in the codebase have been updated to usecrypto_data_hash/3.432-439: The deprecation notice for
sha_hash/2is clear and points users to the newcrypto_data_hash/3function. This is good practice when deprecating functions.
ccamel
force-pushed
the
feat/crypto-data-hash-predicate
branch
from
c2ba248
to
b5a8320
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (11)
- docs/predicate/predicates.md (3 hunks)
- x/logic/interpreter/registry.go (2 hunks)
- x/logic/predicate/chain.go (1 hunks)
- x/logic/predicate/crypto.go (6 hunks)
- x/logic/predicate/crypto_test.go (4 hunks)
- x/logic/predicate/util.go (3 hunks)
- x/logic/predicate/util_test.go (3 hunks)
- x/logic/util/crypto.go (2 hunks)
- x/logic/util/crypto_enum.go (1 hunks)
- x/logic/util/prolog.go (3 hunks)
- x/logic/util/prolog_test.go (1 hunks)
Files skipped from review due to trivial changes (2)
- x/logic/interpreter/registry.go
- x/logic/predicate/chain.go
Additional comments: 24
x/logic/util/crypto_enum.go (1)
- 1-73: The new
HashAlgenum type and its related functions and variables are well defined. The error handling for invalidHashAlgvalues is also properly implemented. The code is clear, maintainable, and follows best practices.x/logic/predicate/util.go (1)
- 18-26: The new atoms for encoding options are well defined and follow the existing pattern.
x/logic/util/prolog_test.go (1)
- 23-31: The test cases where the
optionsisnilor an empty list are redundant as they both represent an empty list. Consider removing one of them to reduce redundancy.- { - option: engine.NewAtom("foo"), - options: nil, - wantResult: nil, - wantError: nil, - },x/logic/util/prolog.go (4)
13-19: The introduction of
AtomEmptyListis a good way to represent an empty list. This will help in maintaining consistency across the codebase.58-67: The
IsListfunction has been updated to handle bothengine.Compoundandengine.Atomtypes. This is a good change as it makes the function more versatile.69-75: The
IsEmptyListfunction has been updated to handle bothengine.Compoundandengine.Atomtypes. This is a good change as it makes the function more versatile.100-105: The
GetOptionfunction now returnsnilif the resolved term is an empty list. This is a good change as it makes the function more robust and prevents unnecessary processing of empty lists.x/logic/predicate/util_test.go (3)
102-113: The new test case for UTF-8 encoding is correctly implemented and should cover the new functionality adequately.
126-129: The error message for an invalid encoding option has been updated to include the new "utf8" option. This is a good practice as it provides more information to the user about the possible values.
168-174: The
TermToBytesfunction signature has been correctly updated to include the newdefaultValueparameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.x/logic/util/crypto.go (5)
1-1: The
go:generatedirective is used to generate enum string methods. Ensure that thego-enumtool is installed and used correctly.31-48: The
HashAlgtype andHashermethod are introduced to support multiple hash algorithms. This is a good practice as it allows for easy extension of supported algorithms in the future.1-54: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [50-66]
The
VerifySignaturefunction now accepts aKeyAlgtype instead ofAlg. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.
70-79: The
Hashfunction is introduced to hash data using the given algorithm. This is a good practice as it allows for easy extension of supported algorithms in the future.67-83: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [81-95]
The
verifySignatureWithCurvefunction is used to verify the ASN1 signature of a message with a given public key using a specified elliptic curve. This is a good practice as it allows for easy extension of supported algorithms in the future.docs/predicate/predicates.md (3)
163-178: The signature and example for
chain_id/1have been updated. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.180-222: The new
crypto_data_hash/3predicate has been introduced with examples. This function is intended to replace the deprecatedsha_hash/2function. Ensure that all calls tosha_hash/2have been replaced withcrypto_data_hash/3throughout the codebase.432-439: The
sha_hash/2function has been deprecated. Ensure that there are no remaining calls to this function in the codebase.x/logic/predicate/crypto.go (4)
36-50: The deprecated
SHAHashfunction only supportsengine.Atomtype for data. The newCryptoDataHashfunction should handle more types for better versatility.209-213: The
EDDSAVerifyfunction has been updated to use a new typeutil.KeyAlginstead ofutil.Alg. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.249-253: The
ECDSAVerifyfunction has been updated to use a new typeutil.KeyAlginstead ofutil.Alg. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.307-322: The helper function
getOptionAsAtomWithDefaulthas been added. This function retrieves the value of the first option with the given name in the given options. If the option is not found, it returns a default value. This function improves code readability and reusability.x/logic/predicate/crypto_test.go (2)
23-23: The function name has been updated to reflect the broader scope of the tests. Ensure that all references to this function have been updated accordingly.
193-193: The new
crypto_data_hashfunction has been registered for use in the tests. Ensure that the function is correctly implemented and that it behaves as expected in all scenarios.
Oh yes sure. we can remove it. Want to take care of it 😛 |
If it's ok for you you can merge :) |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- x/logic/interpreter/registry.go (1 hunks)
- x/logic/predicate/crypto.go (5 hunks)
- x/logic/predicate/crypto_test.go (3 hunks)
Additional comments: 14
x/logic/interpreter/registry.go (1)
- 109-115: The registry map has been updated to include the new
crypto_data_hash/3predicate and remove the deprecatedsha_hash/2. Ensure that all references to the oldsha_hash/2predicate are updated to use the newcrypto_data_hash/3predicate. Additionally, verify that the new predicate is implemented correctly and that all necessary unit tests are in place to validate its functionality.x/logic/predicate/crypto_test.go (5)
23-24: Renaming the test function to
TestCryptoOperationsreflects the broader scope of the cryptographic operations being tested. This is a good practice as it makes the purpose of the test clearer.30-140: The test cases have been updated to include a
wantSuccessboolean. This is a good practice as it explicitly states the expected outcome of the test, making the test cases easier to understand and maintain.33-37, 38-43, ...:
The test cases forhex_bytespredicate are well-structured and cover various scenarios, including success and error cases. This thorough testing ensures that the predicate behaves as expected in different situations.
120-122: The test case for an invalid algorithm in
crypto_data_hashis important for ensuring that the function handles incorrect inputs gracefully and provides a clear error message.148-153: The registration of the new
crypto_data_hashpredicate in the Prolog interpreter is necessary for the test cases to use it. This change is consistent with the introduction of the new predicate.147-153: It's important to verify that the
Compilemethod does not return an error before proceeding with the tests. This ensures that the Prolog program is syntactically correct and can be executed.Convey blocks (nested):
The use of nestedConveyblocks provides a clear structure for the tests, allowing for setup, execution, and assertions to be grouped logically. This makes the tests easier to follow and maintain.Error handling in Convey blocks:
The error handling within theConveyblocks is consistent and thorough, checking for both the presence and absence of errors as appropriate, and comparing error messages when errors are expected.Overall, the test cases are well-structured, cover a wide range of scenarios, and follow good practices for error handling and assertions. The changes made in the pull request are consistent with the introduction of the new
crypto_data_hashpredicate and the deprecation ofsha_hash. The tests are likely to be effective in catching regressions and ensuring the correctness of the cryptographic predicates.x/logic/predicate/crypto.go (8)
60-60: The
CryptoDataHashfunction is using aDelayfunction to return a promise. This is a common pattern in Prolog implementations to handle computations that should be executed later. Ensure that the delayed execution context is properly managed and that it does not lead to any race conditions or state inconsistencies.61-61: The
getOptionAsAtomWithDefaultfunction is used to retrieve the algorithm option with a default value of "sha256". This is a good use of a helper function to handle default values and option parsing, which can improve code maintainability.72-72: The
TermToBytesfunction is called withAtomUtf8as a parameter, butAtomUtf8is not defined within the provided code snippet. Ensure thatAtomUtf8is properly defined and accessible within this context.175-175: The
EDDSAVerifyfunction is a wrapper around a more genericxVerifyfunction, passing in specific parameters for the EdDSA algorithm. This is a good example of code reuse and maintaining a clean API surface.---end hunk 1---
---start hunk 2---
- 215-215: Similar to the
EDDSAVerifyfunction,ECDSAVerifyis also a wrapper around thexVerifyfunction. It's good to see consistency in how these verification functions are implemented.---end hunk 2---
---start hunk 3---
235-240: The use of
slices.IndexFuncand the subsequent error handling is a good way to validate the algorithm type against a list of supported algorithms. This ensures that only valid algorithms are used for the verification process.242-255: The
TermToBytesfunction is used multiple times with different encoding options. It's important to ensure that theTermToBytesfunction is robust and handles all possible encodings correctly. Additionally, the error messages are clear and provide sufficient information for debugging.---end hunk 3---
---start hunk 4---
- 270-285: The
getOptionAsAtomWithDefaulthelper function is well-implemented, providing a clear and reusable way to handle options with default values. The error handling is consistent and will provide useful information in case of failure.---end hunk 4---
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/logic/predicate/crypto.go (5 hunks)
Additional comments: 8
x/logic/predicate/crypto.go (8)
55-56: The function signature for
CryptoDataHashhas been updated to include anoptionsparameter, which allows for specifying the hashing algorithm and data encoding. This is a significant change that should be clearly documented and communicated to users, as it may require updates to existing code that uses this function.61-70: The error handling within
CryptoDataHashis robust, providing clear error messages when an invalid algorithm is specified. This is good practice as it aids in debugging and user comprehension when errors occur.72-74: The
TermToBytesfunction is used to decode the data term based on the specified encoding options. It's important to ensure that this function handles all possible encodings correctly and that it's been updated to support the new "utf8" encoding option mentioned in the pull request summary.82-82: The
BytesToListfunction is used to convert the result of the hashing operation into a Prolog list. It's important to ensure that this conversion is done correctly and efficiently, as improper handling of data types can lead to subtle bugs.---end hunk 0---
---start hunk 1---
- 174-175: The
EDDSAVerifyfunction now uses a newutil.KeyAlgtype for specifying cryptographic algorithms. This change should be verified for compatibility with existing code and the correct functioning of the verification process.---end hunk 1---
---start hunk 2---
- 214-215: The
ECDSAVerifyfunction has been updated similarly toEDDSAVerify, using the newutil.KeyAlgtype. As withEDDSAVerify, it's important to verify that this change integrates well with the rest of the system and that the verification process works as expected with the new type.---end hunk 2---
---start hunk 3---
242-255: The
TermToBytesfunction is used multiple times to decode different terms (key, data, signature) based on the specified encoding options. As with theCryptoDataHashfunction, it's important to ensure that this function is robust and handles all specified encodings correctly.257-260: The
util.VerifySignaturefunction is called to perform the actual signature verification. It's important to ensure that this function is implemented correctly and securely, as flaws in signature verification can lead to serious security vulnerabilities.---end hunk 3---
---start hunk 4---
|
@coderabbitai pause |
Introduces the
crypto_data_hash/3predicate, which closely looks like the one provided by swiprolog, with current support forMD5(for convenience),SHA-256andSHA-512. This predicate offers more versatility (in terms of algorithm and encoding) compared to thenow-deprecatednow-removedsha_hash/2predicate.Additionally, this PR addresses a bug related to parsing options for the empty list (
[]), correcting the issue where the empty list was treated as a compound term, instead of an atom.--
Note: The code for the
HashAlgenum type has been generated with abice/go-enum.Summary by CodeRabbit
New Features
crypto_data_hash/3predicate for enhanced cryptographic hashing with multiple algorithms and encoding options.chain_id/1predicate signature tochain_id(?ID)for improved clarity.Improvements
sha_hash/2predicate in favor of the more versatilecrypto_data_hash/3.util.KeyAlgtype for specifying cryptographic algorithms.Bug Fixes
Documentation
crypto_data_hash/3predicate.Refactor
SHAHashtoCryptoDataHashto align with new functionality.Tests
crypto_data_hashand UTF-8 encoding.