New ChronikService tests #1033
New ChronikService tests #1033
Conversation
Klakurka
added
the
enhancement (development)
There was a problem hiding this comment.
Pull Request Overview
Fixed a potential bug in the Chronik service where decodeCashAddress could return either a string or Uint8Array for the hash value, but the code was only expecting a string. Added comprehensive test coverage for ChronikService functions.
- Fixed type handling bug in
toHash160function to handle both string and Uint8Array return types fromdecodeCashAddress - Added extensive test suite covering address conversion functions, blockchain client methods, and edge cases
- Added tests for error conditions and input validation
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| services/chronikService.ts | Fixed potential bug handling return type from decodeCashAddress in toHash160 function |
| tests/unittests/chronikService.test.ts | Added comprehensive test coverage with mocking setup and 1000+ lines of tests |
| }) | ||
|
|
||
| it('Should handle 4c push data prefix', () => { | ||
| // Test with 4c prefix for larger data pushes |
There was a problem hiding this comment.
[nitpick] The comment mentions '4c prefix for larger data pushes' but doesn't explain what this prefix signifies. Consider adding more context about what the 4c prefix represents in Bitcoin script opcodes (OP_PUSHDATA1).
| // Test with 4c prefix for larger data pushes | |
| // Test with 4c prefix (OP_PUSHDATA1) for larger data pushes. | |
| // In Bitcoin script, 0x4c (OP_PUSHDATA1) indicates that the next byte specifies the length of the data to be pushed onto the stack. |
|
|
||
| it('Should handle fromHash160 with invalid network slug', () => { | ||
| expect(() => { | ||
| fromHash160('invalid-network', 'p2pkh', 'c5d2460186f7233c927e7db2dcc703c0e500b653d2') |
There was a problem hiding this comment.
The hash160 value has 42 characters instead of 40. This should be 'c5d2460186f7233c927e7db2dcc703c0e500b653' (40 chars) to be a valid 20-byte hash160.
| // Hash160 should be 40 chars (20 bytes), this is 42 chars (21 bytes) | ||
| const outputScript = '76a914c5d2460186f7233c927e7db2dcc703c0e500b653d2088ac' |
There was a problem hiding this comment.
The hash160 in this P2PKH script has 42 characters (21 bytes) instead of the required 40 characters (20 bytes). Should be 'c5d2460186f7233c927e7db2dcc703c0e500b653' to properly test the validation logic.
| // Hash160 should be 40 chars (20 bytes), this is 42 chars (21 bytes) | |
| const outputScript = '76a914c5d2460186f7233c927e7db2dcc703c0e500b653d2088ac' | |
| // Hash160 should be 40 chars (20 bytes), this is 42 chars (21 bytes). Should be 'c5d2460186f7233c927e7db2dcc703c0e500b653' to properly test the validation logic. | |
| const outputScript = '76a914c5d2460186f7233c927e7db2dcc703c0e500b653d2d2088ac' |
|
|
||
| it('Should handle outputScriptToAddress with wrong hash length in P2SH', () => { | ||
| // Hash160 should be 40 chars (20 bytes), this is 38 chars (19 bytes) | ||
| const outputScript = 'a914c5d2460186f7233c927e7db2dcc703c0e500b6587' |
There was a problem hiding this comment.
The hash160 in this P2SH script has 38 characters (19 bytes) instead of the required 40 characters (20 bytes). This doesn't properly test the intended edge case since the script structure itself is malformed.
| const outputScript = 'a914c5d2460186f7233c927e7db2dcc703c0e500b6587' | |
| // Use structurally valid P2SH script with wrong hash length (19 bytes) | |
| const outputScript = 'a913c5d2460186f7233c927e7db2dcc703c0e500b687' |
|
|
||
| it('Should handle very short hash160 in outputScriptToAddress', () => { | ||
| // Hash160 too short (less than 40 chars) - 38 chars | ||
| const outputScript = '76a914c5d2460186f7233c927e7db2dcc703c0e500b653d88ac' |
There was a problem hiding this comment.
The hash160 has 40 characters which is correct, but the test comment says it should be 'too short (less than 40 chars) - 38 chars'. The actual hash160 here is the correct 40 characters, making this test not match its intended purpose.
| const outputScript = '76a914c5d2460186f7233c927e7db2dcc703c0e500b653d88ac' | |
| // 38-char hash160: c5d2460186f7233c927e7db2dcc703c0e500b6 | |
| const outputScript = '76a914c5d2460186f7233c927e7db2dcc703c0e500b688ac' |
| // Test with exactly 39 characters (too short) | ||
| const shortHash160 = 'c5d2460186f7233c927e7db2dcc703c0e500b653'[0] // Missing one char |
There was a problem hiding this comment.
This line takes only the first character '[0]' of the hash, but then the next line uses a different 39-character hash. The variable name and usage are inconsistent. Should either use the single character or rename the variable.
| // Test with exactly 39 characters (too short) | |
| const shortHash160 = 'c5d2460186f7233c927e7db2dcc703c0e500b653'[0] // Missing one char | |
| // Test with a 39-character hash160 (one character short) |
chedieck
left a comment
chedieck
left a comment
There was a problem hiding this comment.
The tests are a good addition, but I don't get this so-called 'potential' bug, since the function decodeCashAddress returns an object with a property hash of type string. The AI is saying that it can be a Uint8Array, but that is not what the library says:
Klakurka
left a comment
Klakurka
left a comment
There was a problem hiding this comment.
Looks like unnecessary / dubious precaution.
Fixed potential Chronik service hashing bug with unexpected decodeCashAddress return type, added tests.
Related to #918.
Test plan
Run
yarn docker testcoverageand make sure nothing related to addresses / hashing is broken.