Sync fork with upstream Merkeleon/php-cryptocurrency-address-validation#5
Open
yoav-ebp wants to merge 54 commits into
Open
Sync fork with upstream Merkeleon/php-cryptocurrency-address-validation#5yoav-ebp wants to merge 54 commits into
yoav-ebp wants to merge 54 commits into
Conversation
Update README.md
…g-some-internal-stuff Improve codebase and add Laravel support
Add new driver
[FIX] update CBOR to make compatible with Laravel 10
…-btc-bch Make validation for BTC and BCH soften
[FEATURE] Add GH action with tests
…pport feature: long p-address support
…ddresses-support
fix: driver config construction from cache
…dation Syncs the fork with upstream master to bring in: - bech32m address support - LTC testnet validation fix - BCH long p-address support - Driver config construction fix - Various bug fixes Note: Accepted upstream's composer.json requirements. Review may be needed to ensure compatibility with our PHP 8.1 requirement.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
18 issues found across 76 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/Utils/Base32Decoder.php">
<violation number="1" location="src/Utils/Base32Decoder.php:107">
P2: Incorrect PHPDoc return type. GMP functions return `\GMP` objects in PHP 5.6+, not resources. The return type should be `@return \GMP`.</violation>
</file>
<file name="src/Drivers/KeccakDriver.php">
<violation number="1" location="src/Drivers/KeccakDriver.php:15">
P1: check() computes a checksum before validating the input format/length. Invalid addresses can trigger string offset warnings/errors inside toChecksum() instead of cleanly returning false. Add a format/length guard before calling toChecksum().</violation>
<violation number="2" location="src/Drivers/KeccakDriver.php:22">
P2: Use a prefix-only, case-insensitive strip for the 0x prefix; str_replace removes "0x" anywhere and misses "0X".</violation>
</file>
<file name="src/Validator.php">
<violation number="1" location="src/Validator.php:36">
P0: Calling `valid()` on a Generator starts its execution. When the generator is subsequently passed to `getDriver()` which uses `foreach`, PHP will throw `Cannot rewind a generator that was already run` because `foreach` internally calls `rewind()` on iterators.
Consider checking if `$this->options` is empty before creating the generator, or convert to array first.</violation>
</file>
<file name="src/Drivers/KeccakStrictDriver.php">
<violation number="1" location="src/Drivers/KeccakStrictDriver.php:7">
P3: Remove unused imports (`InvalidArgumentException`, `is_string`, `strpos`) to avoid dead code and potential lint/static-analysis failures.</violation>
</file>
<file name="src/Drivers/XrpXAddressDriver.php">
<violation number="1" location="src/Drivers/XrpXAddressDriver.php:24">
P1: The `check()` method unconditionally returns `true`, bypassing all checksum validation. The parent class `XrpBase58Driver` properly validates addresses via `$this->getVersion($address)`. This override means any string matching the regex will be accepted as a valid XRP X-address, even with invalid checksums. For a cryptocurrency address validation library, this could result in funds sent to malformed addresses.</violation>
</file>
<file name="src/Drivers/Base32Driver.php">
<violation number="1" location="src/Drivers/Base32Driver.php:48">
P1: Case-sensitivity bug: `$hasPrefix` is determined before lowercasing the address. Laravel's `Str::contains` is case-sensitive, so if `$this->options` keys are lowercase (e.g., `bitcoincash:`) and the address is uppercase (e.g., `BITCOINCASH:qp...`), the prefix check will incorrectly return false. Move the `strtolower()` call before the prefix check.</violation>
</file>
<file name="src/Utils/HexDecoder.php">
<violation number="1" location="src/Utils/HexDecoder.php:27">
P2: Initialize `$dec` as string '0' for consistency with BCMath operations and the method's return type. This also ensures correct return type if input is empty.</violation>
<violation number="2" location="src/Utils/HexDecoder.php:43">
P1: Using standard comparison operator `>` with BCMath strings can lose precision for large cryptocurrency values. Use `bccomp($dec, '0') > 0` instead to safely compare arbitrary-precision numbers.</violation>
</file>
<file name="src/Utils/Bech32Decoder.php">
<violation number="1" location="src/Utils/Bech32Decoder.php:50">
P1: decodeRaw() is public and bypasses the 90-character Bech32 length check (unlike decode()). Since it’s called directly (e.g., CardanoDriver), very long inputs can cause excessive memory/CPU usage during unpack()/looping. Add the same max-length guard in decodeRaw() (or make decodeRaw non-public and migrate callers).</violation>
</file>
<file name="src/Utils/Base58Decoder.php">
<violation number="1" location="src/Utils/Base58Decoder.php:26">
P1: Missing validation for invalid Base58 characters. `strpos()` returns `false` when a character is not in the alphabet, and casting `false` to string yields `""` (treated as 0), silently ignoring invalid input. This could cause malformed addresses to be incorrectly validated.</violation>
</file>
<file name="README.md">
<violation number="1" location="README.md:8">
P1: Leftover git merge conflict marker `=======` should be removed. This will render as visible text in the README.</violation>
</file>
<file name="src/Contracts/Validator.php">
<violation number="1" location="src/Contracts/Validator.php:16">
P2: Missing `use` statement for `AddressValidationException`. The exception is in `Merkeleon\PhpCryptocurrencyAddressValidation\Exception` namespace, but the `@throws` annotation references it without importing it. IDEs and static analysis tools won't properly resolve this type reference.</violation>
</file>
<file name="src/Utils/KeccakDecoder.php">
<violation number="1" location="src/Utils/KeccakDecoder.php:275">
P1: Bug: Using `(int)` cast instead of `ord()` produces wrong byte value. In PHP, `(int) $char` parses the character as a decimal string (returning 0 for non-digits), while `ord($char)` returns the ASCII byte value (0-255). This will cause incorrect Keccak hash output on 32-bit systems.</violation>
</file>
<file name="src/Drivers/Bech32Driver.php">
<violation number="1" location="src/Drivers/Bech32Driver.php:29">
P2: The return value of `preg_match()` is not checked before accessing `$match[2]`. If the pattern doesn't match, accessing `$match[2]` will cause an undefined array key error. Unlike `match()` which correctly checks `=== 1`, `check()` ignores the return value.</violation>
</file>
<file name="src/Exception/AddressValidationException.php">
<violation number="1" location="src/Exception/AddressValidationException.php:12">
P2: Grammar errors in exception message: "address have wrong encoding" should be "address has wrong encoding", and "does not matched" should be "does not match".</violation>
</file>
<file name=".github/workflows/tests.yml">
<violation number="1" location=".github/workflows/tests.yml:20">
P1: Using `@master` branch reference is a security risk. Pin to a specific version tag (e.g., `@v2`) or commit SHA to ensure reproducible and secure builds. The master branch can change at any time, potentially breaking builds or introducing vulnerabilities.</violation>
</file>
<file name="src/Drivers/EosDriver.php">
<violation number="1" location="src/Drivers/EosDriver.php:11">
P1: Regex allows 13-character addresses but EOS account names are limited to 12 characters maximum. The second pattern `{12}[a-j1-5]` matches 13 characters total. Should be `{11}` to match 12 characters.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| $drivers = $this->getDrivers(); | ||
| // if there is no drivers we force address to be valid | ||
| if (null === $drivers || !$drivers->valid()) { |
There was a problem hiding this comment.
P0: Calling valid() on a Generator starts its execution. When the generator is subsequently passed to getDriver() which uses foreach, PHP will throw Cannot rewind a generator that was already run because foreach internally calls rewind() on iterators.
Consider checking if $this->options is empty before creating the generator, or convert to array first.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/Validator.php, line 36:
<comment>Calling `valid()` on a Generator starts its execution. When the generator is subsequently passed to `getDriver()` which uses `foreach`, PHP will throw `Cannot rewind a generator that was already run` because `foreach` internally calls `rewind()` on iterators.
Consider checking if `$this->options` is empty before creating the generator, or convert to array first.</comment>
<file context>
@@ -0,0 +1,92 @@
+
+ $drivers = $this->getDrivers();
+ // if there is no drivers we force address to be valid
+ if (null === $drivers || !$drivers->valid()) {
+ return true;
+ }
</file context>
Comment on lines
+15
to
+17
| $address = $this->toChecksum($address); | ||
|
|
||
| return parent::check($address); |
There was a problem hiding this comment.
P1: check() computes a checksum before validating the input format/length. Invalid addresses can trigger string offset warnings/errors inside toChecksum() instead of cleanly returning false. Add a format/length guard before calling toChecksum().
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/Drivers/KeccakDriver.php, line 15:
<comment>check() computes a checksum before validating the input format/length. Invalid addresses can trigger string offset warnings/errors inside toChecksum() instead of cleanly returning false. Add a format/length guard before calling toChecksum().</comment>
<file context>
@@ -0,0 +1,38 @@
+{
+ public function check(string $address): bool
+ {
+ $address = $this->toChecksum($address);
+
+ return parent::check($address);
</file context>
Suggested change
| $address = $this->toChecksum($address); | |
| return parent::check($address); | |
| if (!preg_match('/^(0x)?[0-9a-fA-F]{40}$/', $address)) { | |
| return false; | |
| } | |
| $address = $this->toChecksum($address); | |
| return parent::check($address); |
|
|
||
| public function check(string $address): bool | ||
| { | ||
| return true; |
There was a problem hiding this comment.
P1: The check() method unconditionally returns true, bypassing all checksum validation. The parent class XrpBase58Driver properly validates addresses via $this->getVersion($address). This override means any string matching the regex will be accepted as a valid XRP X-address, even with invalid checksums. For a cryptocurrency address validation library, this could result in funds sent to malformed addresses.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/Drivers/XrpXAddressDriver.php, line 24:
<comment>The `check()` method unconditionally returns `true`, bypassing all checksum validation. The parent class `XrpBase58Driver` properly validates addresses via `$this->getVersion($address)`. This override means any string matching the regex will be accepted as a valid XRP X-address, even with invalid checksums. For a cryptocurrency address validation library, this could result in funds sent to malformed addresses.</comment>
<file context>
@@ -0,0 +1,26 @@
+
+ public function check(string $address): bool
+ {
+ return true;
+ }
+}
</file context>
Suggested change
| return true; | |
| return $this->getVersion($address) !== null; |
Comment on lines
+48
to
+50
| $hasPrefix = Str::contains($address, array_keys($this->options)); | ||
|
|
||
| $address = strtolower($address); |
There was a problem hiding this comment.
P1: Case-sensitivity bug: $hasPrefix is determined before lowercasing the address. Laravel's Str::contains is case-sensitive, so if $this->options keys are lowercase (e.g., bitcoincash:) and the address is uppercase (e.g., BITCOINCASH:qp...), the prefix check will incorrectly return false. Move the strtolower() call before the prefix check.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/Drivers/Base32Driver.php, line 48:
<comment>Case-sensitivity bug: `$hasPrefix` is determined before lowercasing the address. Laravel's `Str::contains` is case-sensitive, so if `$this->options` keys are lowercase (e.g., `bitcoincash:`) and the address is uppercase (e.g., `BITCOINCASH:qp...`), the prefix check will incorrectly return false. Move the `strtolower()` call before the prefix check.</comment>
<file context>
@@ -0,0 +1,114 @@
+ public function check(string $address, array $networks = []): bool
+ {
+ try {
+ $hasPrefix = Str::contains($address, array_keys($this->options));
+
+ $address = strtolower($address);
</file context>
Suggested change
| $hasPrefix = Str::contains($address, array_keys($this->options)); | |
| $address = strtolower($address); | |
| $address = strtolower($address); | |
| $hasPrefix = Str::contains($address, array_keys($this->options)); |
| $last = (int)bcmod($dec, '16', 8); | ||
| $hex = dechex($last) . $hex; | ||
| $dec = bcdiv(bcsub($dec, (string)$last, 8), '16', 8); | ||
| } while ($dec > 0); |
There was a problem hiding this comment.
P1: Using standard comparison operator > with BCMath strings can lose precision for large cryptocurrency values. Use bccomp($dec, '0') > 0 instead to safely compare arbitrary-precision numbers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/Utils/HexDecoder.php, line 43:
<comment>Using standard comparison operator `>` with BCMath strings can lose precision for large cryptocurrency values. Use `bccomp($dec, '0') > 0` instead to safely compare arbitrary-precision numbers.</comment>
<file context>
@@ -0,0 +1,47 @@
+ $last = (int)bcmod($dec, '16', 8);
+ $hex = dechex($last) . $hex;
+ $dec = bcdiv(bcsub($dec, (string)$last, 8), '16', 8);
+ } while ($dec > 0);
+
+ return $hex;
</file context>
Suggested change
| } while ($dec > 0); | |
| } while (bccomp($dec, '0') > 0); |
| $hex = substr($hex, 2); | ||
| } | ||
|
|
||
| $dec = 0; |
There was a problem hiding this comment.
P2: Initialize $dec as string '0' for consistency with BCMath operations and the method's return type. This also ensures correct return type if input is empty.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/Utils/HexDecoder.php, line 27:
<comment>Initialize `$dec` as string '0' for consistency with BCMath operations and the method's return type. This also ensures correct return type if input is empty.</comment>
<file context>
@@ -0,0 +1,47 @@
+ $hex = substr($hex, 2);
+ }
+
+ $dec = 0;
+ $len = strlen($hex);
+ for ($i = 1; $i <= $len; $i++) {
</file context>
Suggested change
| $dec = 0; | |
| $dec = '0'; |
| * | ||
| * @return void | ||
| * | ||
| * @throws AddressValidationException |
There was a problem hiding this comment.
P2: Missing use statement for AddressValidationException. The exception is in Merkeleon\PhpCryptocurrencyAddressValidation\Exception namespace, but the @throws annotation references it without importing it. IDEs and static analysis tools won't properly resolve this type reference.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/Contracts/Validator.php, line 16:
<comment>Missing `use` statement for `AddressValidationException`. The exception is in `Merkeleon\PhpCryptocurrencyAddressValidation\Exception` namespace, but the `@throws` annotation references it without importing it. IDEs and static analysis tools won't properly resolve this type reference.</comment>
<file context>
@@ -0,0 +1,19 @@
+ *
+ * @return void
+ *
+ * @throws AddressValidationException
+ */
+ public function validate(?string $address): void;
</file context>
| $address = strtolower($address); | ||
|
|
||
| $expr = $this->getPattern(); | ||
| preg_match($expr, $address, $match); |
There was a problem hiding this comment.
P2: The return value of preg_match() is not checked before accessing $match[2]. If the pattern doesn't match, accessing $match[2] will cause an undefined array key error. Unlike match() which correctly checks === 1, check() ignores the return value.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/Drivers/Bech32Driver.php, line 29:
<comment>The return value of `preg_match()` is not checked before accessing `$match[2]`. If the pattern doesn't match, accessing `$match[2]` will cause an undefined array key error. Unlike `match()` which correctly checks `=== 1`, `check()` ignores the return value.</comment>
<file context>
@@ -0,0 +1,52 @@
+ $address = strtolower($address);
+
+ $expr = $this->getPattern();
+ preg_match($expr, $address, $match);
+
+ [$hrpGot, $data] = (new Bech32Decoder())->decode($address);
</file context>
Suggested change
| preg_match($expr, $address, $match); | |
| if (preg_match($expr, $address, $match) !== 1) { | |
| return false; | |
| } |
| public function __construct(string $chain, string $notValidAddress, bool $matchedPattern) | ||
| { | ||
| $text = "Incorrect {$chain} address [{$notValidAddress}]"; | ||
| $text .= $matchedPattern ? ": address have wrong encoding" : ": address does not matched pattern"; |
There was a problem hiding this comment.
P2: Grammar errors in exception message: "address have wrong encoding" should be "address has wrong encoding", and "does not matched" should be "does not match".
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/Exception/AddressValidationException.php, line 12:
<comment>Grammar errors in exception message: "address have wrong encoding" should be "address has wrong encoding", and "does not matched" should be "does not match".</comment>
<file context>
@@ -0,0 +1,15 @@
+ public function __construct(string $chain, string $notValidAddress, bool $matchedPattern)
+ {
+ $text = "Incorrect {$chain} address [{$notValidAddress}]";
+ $text .= $matchedPattern ? ": address have wrong encoding" : ": address does not matched pattern";
+ parent::__construct($text);
+ }
</file context>
Suggested change
| $text .= $matchedPattern ? ": address have wrong encoding" : ": address does not matched pattern"; | |
| $text .= $matchedPattern ? ": address has wrong encoding" : ": address does not match pattern"; |
|
|
||
| namespace Merkeleon\PhpCryptocurrencyAddressValidation\Drivers; | ||
|
|
||
| use InvalidArgumentException; |
There was a problem hiding this comment.
P3: Remove unused imports (InvalidArgumentException, is_string, strpos) to avoid dead code and potential lint/static-analysis failures.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/Drivers/KeccakStrictDriver.php, line 7:
<comment>Remove unused imports (`InvalidArgumentException`, `is_string`, `strpos`) to avoid dead code and potential lint/static-analysis failures.</comment>
<file context>
@@ -0,0 +1,59 @@
+
+namespace Merkeleon\PhpCryptocurrencyAddressValidation\Drivers;
+
+use InvalidArgumentException;
+use Merkeleon\PhpCryptocurrencyAddressValidation\Utils\KeccakDecoder;
+use function intval;
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Syncs this fork with the upstream repository Merkeleon/php-cryptocurrency-address-validation to bring in the latest changes.
Changes from upstream:
Notes
ext-gmp,ext-bcmathrequirementsspomky-labs/cbor-phpto ^3.0Please review these dependency changes to ensure compatibility with our environment.
Summary by cubic
Syncs our fork with upstream to adopt the new driver-based validator, add bech32m support, and Laravel integration. Updates dependencies and tests, and fixes LTC testnet validation plus BCH long p-address handling.
New Features
Migration
Written for commit 7bbe0fe. Summary will update on new commits.