Merge quality-pass - #45
Merged
Merged
Conversation
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- RpcMethodResponse serves canned rippled responses keyed by JSON-RPC method. MockRippledResponse routes by URL path, which the client never varies, so anything reaching rippled through the normal code path could not be mocked. - FeeCalculationTest covers the network fee, the owner reserve that AccountDelete and AMMCreate pay instead, that maxFeeXrp does not cap the reserve but does cap the network fee, and that the EscrowFinish surcharge scales with the fulfillment size. Both special cases were fixed in 2.0.0 without a test. Reverting either fix now breaks two of these. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The test suite ran with four deprecation notices, all from brick/math 0.14 announcing removals in 0.15: - MathUtilities used BigDecimal::getIntegralPart() and getFractionalPart(), which 0.15 removes and 0.16 reintroduces with a different meaning. They are replaced by splitting the decimal's string form, which reproduces both parts including the sign. - DROPS_PER_XRP was the float 1000000.0 although drops are a whole number, so every conversion passed a float into brick/math. - The fee cushion, the load factor and the EscrowFinish multiplier reached brick/math as floats and are converted explicitly. - exactlyDividedBy() is renamed to dividedByExact(). MathUtilitiesTest characterises the three decimal helpers, which had no tests of their own. The expectations were captured from the old implementation before the change, quirks included: the precision of a value below one counts the leading zero, and zero itself has precision 0. The same 13 cases were also run against a real brick/math 0.15 in an isolated project and produce identical results. The constraint stays at <0.15 because hardcastle/buffer caps brick/math at ^0.14, so the full suite cannot be run against it yet. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The eleven free functions of src/Sugar/autofill.php become methods of Hardcastle\XRPL_PHP\Client\Autofiller, which holds the client instead of taking it as a first argument. The free functions remain as @deprecated wrappers that delegate, so existing code keeps working. Fixes two bugs in the AccountDelete blocker check, which was dead twice over: - It was guarded by `!isset($tx['TransactionType'])` rather than a comparison against AccountDelete, so it ran only for transactions without a type and never for the one it exists for. - It counted blockers as `$result['account_objects']['length']`, a JavaScript idiom. In PHP that is an undefined key, so the comparison was `null > 0` and could never be true. JsonRpcClient::autofill() no longer takes the transaction by reference. It was never modified through it - the value was passed on by value internally - and the reference only forced callers to assign the array to a variable first. The method also accepts $signersCount now, which the multi-signing fee path needed but could not be reached from the client. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The eight free functions of src/Sugar/submit.php become methods of Hardcastle\XRPL_PHP\Client\Submitter, which holds the client instead of taking it as a first argument. The free functions remain as @deprecated wrappers that delegate. Fixes getSignedTx(): it returned the tx_blob/hash envelope of Wallet::sign() while every caller expects a transaction array, so submitRequest() found no SigningPubKey and threw "Transaction must be signed". Submitting an unsigned transaction together with a wallet - the documented convenience path and the reason submit() and submitAndWait() take a $wallet at all - therefore always failed. Every example works around it by signing first and passing the blob. The signed blob is now decoded back into an array, the shape the already-signed branch returns. SubmitTest covers isSigned(), getLastLedgerSequence() and isAccountDelete() for both arrays and blobs, the three getSignedTx() paths, and that submit() reaches the ledger with an unsigned transaction and a wallet. The polling loop of submitAndWait() sleeps three seconds per attempt and is left to the integration tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
getXrpBalance(), getBalances() and getTransactions() become methods of AccountReader; getOrderbook() becomes a method of OrderbookReader. Both hold the client instead of taking it as a first argument. The free functions remain as @deprecated wrappers that delegate. Drops formatBalances(), which was declared to return an array but had its entire body commented out, so it raised a TypeError on every call. Nothing referenced it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
getFeeXrp() becomes FeeCalculator::getFeeXrp() and fundWallet() becomes Faucet::fundWallet(). The free functions remain as @deprecated wrappers, and JsonRpcClient no longer imports a single function from Sugar - every method now delegates to a class. Drops two dead stubs from fundWallet.php: getHttpOptions() was never called, and getUpdatedBalance() computed a balance, had an empty if body and returned 0 unconditionally. Neither was referenced anywhere. Faucet::waitForFunding() replaces the inline polling loop and now returns as soon as the balance rises, rather than falling through the remaining attempts. An empty faucet response raises instead of silently continuing with an undefined address. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Psalm reported 812 findings, so nobody read them. Roughly 60 per cent were issue types that make sense for an application and not for a library: - UnusedClass and PossiblyUnused* flag the public surface, whose callers are consumers Psalm cannot see - 158 classes, 95 methods and properties. - ClassMustBeFinal wants 227 classes closed, including BaseTransaction, which every transaction model extends and which a separate Xahau package extends from outside. - MissingOverrideAttribute wants #[\Override], which only exists from PHP 8.3 while this library supports ^8.2 and CI builds 8.2. Suppressing those leaves 179 real findings. They are frozen in psalm-baseline.xml so the run is green and any new finding turns it red - a ratchet rather than a permanent wall of noise. The error level stays at 2, so type errors in new code are still reported in full. Psalm now runs in CI, without which the baseline would not hold. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
custom_currency_codes.php was snake_case and xrpBalance.php camelCase, while every other file in examples/ and examples/internal/ uses hyphens. The numbered prefixes in examples/quickstart/ encode a reading order rather than a spelling and are left alone. Only variable names inside xrp-balance.php still contain xrpBalance; no documentation referenced either filename. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Follows the house conventions from the repository consistency document: - containers/php/ becomes docker/, matching docker/Dockerfile plus a docker-compose.yml in the root. The nested layout implied several services where there is one image. - The three compose files become one. The platform differences were the user the container runs as, a tmpfs mount and how the host is reached; the first now comes from DOCKER_UID/DOCKER_GID in .env, the second is harmless everywhere, and extra_hosts maps host.docker.internal to the host gateway so the same xdebug.ini works on Linux and macOS. docker-compose.override.yml stays as the local escape hatch and remains gitignored. - xdebug-mac.ini and xdebug-linux.ini are gone. Neither compose file ever mounted them - both referenced xdebug.ini - and the Linux one had xdebug commented out entirely. Fixes docker-compose.linux.yml along the way, which mounted php.ini twice and xdebug.ini not at all, so Linux users got no xdebug configuration. That file no longer exists, but the bug would have been carried into the merged one. The license is already MIT in the badge, composer.json and LICENSE.md, and the CI workflow triggers on push without a branch filter, so the master to main rename needs no in-code change here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
submitAndWait() polls the ledger with a three second sleep between attempts, so the unit suite reached everything around it but never the loop itself. The new test funds two accounts from the faucet and drives the whole path: autofill against a live server, signing, submission, and polling until the transaction sits in a validated ledger. It also covers the pre-signed blob form the examples use, and that a transaction without a LastLedgerSequence is refused. The test needs the faucet and takes about 45 seconds, so it carries the group integration-slow and CI excludes it. Run it with `vendor/bin/phpunit --group integration-slow`. It immediately found what the unit tests could not: rippled sends base_fee_xrp as a JSON number, so it arrives as a float and brick/math deprecates that. The mocks used a string and therefore never reproduced it. FeeCalculator now converts explicitly, and the mocks use a number, so the unit suite would catch a regression on its own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The class extends BaseResponse but imported BaseRequest, so autoloading it raised "Class BaseResponse not found" and it could never be instantiated. Every other response class in the namespace imports BaseResponse. Found by scanning all 233 classes under src/ for loadability; it is the only one affected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Class docblock coverage goes from 70 to 100 per cent. The 69 that had none, or only an empty frame, now say what the thing is - a Hash160 is not simply "a Hash160" but the width the order book directory fields use, and an Issue is an asset without an amount, in three shapes. SerializedType now documents the contract its 29 subclasses share: what fromParser(), fromJson(), toJson() and toBytes() mean, that the round trip is what has to hold rather than symmetry with PHP types, and that only STObject and STArray resolve field names. That covers roughly 120 subclass methods which would otherwise each have repeated the same three sentences. Also fills the gaps in the classes added earlier on this branch: Autofiller and Submitter carried several docblocks over verbatim from the free functions, and those were tag frames without a description. Wording follows ripple-binary-codec where the type is a direct port. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Public method coverage goes from 18 to 47 per cent. What was added are the methods where the name alone leaves a question open: - JsonRpcClient and Wallet, the surface most callers touch. getBalances() and getTransactions() page through the ledger, which is worth knowing before calling them in a loop. - The address codec and CoreUtilities, where the distinction between a classic address and an X-address decides whether a destination tag survives. - BinaryParser and BinarySerializer, whose field headers and length prefixes are variable width - not obvious from readFieldHeader() alone. - The two key pair services, including why secp256k1 goes through a root key and a sequence while Ed25519 does not. Left undocumented on purpose: 78 trivial accessors, 37 request and response models whose class docblock already links the xrpl.org page, and the fromParser/fromJson/toJson triads of the codec types, whose shared contract is described once on SerializedType. The @PARAM on the abstract SerializedType::fromJson() is gone again: several subclasses widen the parameter to string|int, and pinning it to string made Psalm call their is_string() checks redundant. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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
Quality pass across the library: the Sugar functions get an object form, the static analysis becomes usable, the dev setup follows the house conventions, and the documentation gaps are closed.
Additive throughout — every free function stays, marked
@deprecatedand delegating to the new class. 2.2.0, not a major.Sugar to OOP
Six classes that hold the client instead of taking it as a first argument:
Autofillerautofill.phpSubmittersubmit.phpAccountReadergetXrpBalance,getBalances,getTransactionsOrderbookReadergetOrderbookFeeCalculatorgetFeeXrpFaucetfundWalletJsonRpcClientno longer imports a single function fromSugar. The free functions are one-line delegations — no duplicated logic.JsonRpcClient::autofill()also drops its by-reference parameter. It was never written through, and only forced callers to assign the array to a variable first. It now accepts$signersCount, which the multi-signing fee path needed but could not be reached from the client.Fixed
Seven bugs, all in code no test touched before:
getSignedTx()returned['tx_blob','hash']instead of a transaction arraysubmit()andsubmitAndWait()with a wallet always threw "Transaction must be signed" — the$walletparameter was unusable, every example works around it!isset($tx['TransactionType'])$objects['length']DROPS_PER_XRPwas a floatbase_fee_xrparrives from rippled as a JSON numberdocker-compose.linux.ymlmountedphp.initwice,xdebug.inineverAccountOffersResponseimportedBaseRequestwhile extendingBaseResponseEvery fix is guarded by a test that was verified to fail when the fix is reverted. The three dead stubs
formatBalances(),getUpdatedBalance()andgetHttpOptions()are gone; two of them raised aTypeErroron call.Tests
626 unit tests, up from 624 — but the interesting number is what became testable.
RpcMethodResponseserves canned rippled responses keyed by JSON-RPC method; the previous mock routed by URL path, which the client never varies, so nothing reaching rippled through the normal code path could be mocked at all.SubmitAndWaitTestruns against the Testnet, funds two accounts and drives autofill, signing, submission and the polling loop. It carries the groupintegration-slowand CI excludes it:vendor/bin/phpunit --group integration-slowIt found the base_fee_xrp float immediately — a case the mocks could not reproduce because they used a string. The mocks now use a number, so the unit suite catches a regression on its own.
Static analysis
Psalm reported 812 findings, so nobody read them. About 60 per cent were issue types that suit an application and not a library: UnusedClass and PossiblyUnused* flag the public surface, whose callers Psalm cannot see, and ClassMustBeFinal wants BaseTransaction closed — the class every transaction model extends, including those in a separate Xahau package. MissingOverrideAttribute wants #[\Override], which needs PHP 8.3 while this library supports ^8.2.
Suppressing those leaves 179 real findings, frozen in psalm-baseline.xml. The run is green and any new finding turns it red. Psalm now runs in CI, without which the baseline would not hold. Error level stays at 2.
The ratchet works: adding a @PARAM to the abstract SerializedType::fromJson() during the documentation pass produced 28 errors, because several subclasses widen the parameter to string|int and their is_string() checks became redundant. Caught before it was committed.
Conventions
Follows the repository consistency document:
Documentation
Counted as "has a describing docblock"
Deliberately left out: 78 trivial accessors, 37 request/response models whose class docblock already links the xrpl.org page, and the roughly 120
fromParser/fromJson/toJsonmethods of the codec types, whose shared contract is documented once on SerializedType.