Send requests through an internal transport and test the shape of eve…

CHANGELOG.md

@@ -24,6 +24,7 @@ Entries for 2.2.3 and earlier were reconstructed from the git history in October
 - `.editorconfig` and `.gitattributes`.
 - Dev tooling: PHPUnit 9.6, PHPStan (level 6, with a baseline for the existing code) and PHP-CS-Fixer (PSR-12).
 - A GitHub Actions CI workflow: `php -l` and PHPUnit on PHP 7.4 to 8.5 with lowest and highest dependencies, plus PHPStan and a coding-standard check.
+- Request-shape tests for every public request method, checking the HTTP method, URL, headers and body. Requests go through an internal transport that tests replace with a recorder, so the test suite no longer touches the network.
 - Composer scripts: `composer test`, `composer analyse`, `composer cs` and `composer cs:fix`.
 - `homepage` and `support` (issues, source, security policy) in `composer.json`.
 
@@ -32,6 +33,7 @@ Entries for 2.2.3 and earlier were reconstructed from the git history in October
 - **`0`, `false` and `''` params are now sent.** Only `null` params are dropped. Before, `chain => 0`, `minConfirms => 0`, `numConfirmations => 0` and every `false` flag were silently left out. If you passed `0` or `false` to mean "leave it unset", pass `null` instead. GET query strings now send booleans as `true`/`false` instead of `1`/`0`.
 - **Errors now come back as `['error' => …, 'status' => …]`.** A cURL failure, a non-2xx status or an invalid JSON body used to return `null` or BitGo's raw error body. Any other fields BitGo sent are kept, so `isset($r['error'])` checks keep working. `status` is the HTTP status, or `0` if no response arrived. `buildTransaction()` and `signBuildTransaction()` return the same fields as an object.
 - Amounts in `sendTransaction()`, `sendTransactionToMany()` and `buildTransaction()` accept an `int` or a string of digits, and are always sent as strings, so 18-decimal amounts no longer overflow `PHP_INT_MAX`. `toSatoshi()` accepts strings and no longer breaks under locales that use a decimal comma. `toBTC()` no longer converts through a float.
+- Requests without params send `{}` as the JSON body instead of `[]`, and GET requests without params no longer end the URL with `?`.
 - Requests no longer force IPv4. cURL uses IPv4 or IPv6 as the system allows. A local address bound with `setRequestIP()` decides the IP version.
 - `BitGoSDK::setRequestIP()` returns `true` if the IP was applied, and `false` on testnet or for an invalid IP.
 - **TLS certificates are checked against the system CA store.** The bundled 2022 `cacert.pem` is gone. If PHP finds no system CA store, the Mozilla bundle from the new `composer/ca-bundle` dependency is used.

CONTRIBUTING.md

@@ -74,6 +74,8 @@ CI runs these checks on every pull request. PHPUnit and `php -l` run on PHP 7.4
 
    PHPStan runs at level 6 against a baseline (`phpstan-baseline.neon`) of the errors in the existing code. Don't add new entries to it. When a fix removes an error, regenerate the baseline with `vendor/bin/phpstan analyse --generate-baseline phpstan-baseline.neon`.
 
+   Tests never touch the network. To check what a method sends, install `Tests\Fixtures\RecordingTransport` on the client with `RecordingTransport::on($client)`, then read the captured method, URL, headers and body. You can queue responses or cURL failures on it too. When you add a public method, add a case for it to `SdkRequestShapeTest` or `ExpressRequestShapeTest`. A guard test fails until you do.
+
 2. Exercise your change against BitGo **testnet**. Copy the relevant script from `examples/` into `local_tests/` (which is gitignored), fill in your testnet token and wallet ID there, and run it from the repository root:
 
    ```sh

src/BitGoExpress.php

@@ -16,6 +16,8 @@
 
 use Composer\CaBundle\CaBundle;
 use neto737\BitGoSDK\Enum\CurrencyCode;
+use neto737\BitGoSDK\Internal\CurlTransport;
+use neto737\BitGoSDK\Internal\Transport;
 
 class BitGoExpress
 {
@@ -44,6 +46,7 @@ class BitGoExpress
     private ?string $caFile = null;
     private bool $verifyTls = true;
     private bool $forceIPv4 = false;
+    private ?Transport $transport = null;
 
     /**
      * BitGoExpress Initialization
@@ -672,20 +675,15 @@ private function __execute(string $requestType = 'POST', bool $array = true, boo
         $params = self::withoutNulls($this->params);
         $this->params = [];
 
-        $ch = curl_init();
-        curl_setopt_array($ch, $this->curlOptions($requestType, $params, $authenticate));
+        $this->transport = $this->transport ?? new CurlTransport();
+        $response = $this->transport->send($this->curlOptions($requestType, $params, $authenticate));
 
-        $response = curl_exec($ch);
-        $errno = curl_errno($ch);
-        $error = curl_error($ch);
-        $status = (int) curl_getinfo($ch, CURLINFO_RESPONSE_CODE);
-
-        if ($errno !== 0) {
-            $result = ['error' => $error !== '' ? $error : 'cURL error ' . $errno, 'status' => 0];
+        if ($response['errno'] !== 0) {
+            $result = ['error' => $response['error'] !== '' ? $response['error'] : 'cURL error ' . $response['errno'], 'status' => 0];
             return $array ? $result : (object) $result;
         }
 
-        return self::decodeResponse((string) $response, $status, $array);
+        return self::decodeResponse($response['body'], $response['status'], $array);
     }
 
     /**
@@ -698,12 +696,14 @@ private function __execute(string $requestType = 'POST', bool $array = true, boo
     private function curlOptions(string $requestType, array $params, bool $authenticate = true): array
     {
         $options = [CURLOPT_URL => $this->url];
+        // Cast to an object so that no params are sent as {} rather than [].
+        $body = json_encode((object) $params);
         if ($requestType === 'POST') {
             $options[CURLOPT_POST] = true;
-            $options[CURLOPT_POSTFIELDS] = json_encode($params);
+            $options[CURLOPT_POSTFIELDS] = $body;
         } elseif ($requestType === 'PUT') {
             $options[CURLOPT_CUSTOMREQUEST] = 'PUT';
-            $options[CURLOPT_POSTFIELDS] = json_encode($params);
+            $options[CURLOPT_POSTFIELDS] = $body;
         }
         $options[CURLOPT_SSL_VERIFYPEER] = $this->verifyTls;
         $options[CURLOPT_SSL_VERIFYHOST] = $this->verifyTls ? 2 : 0;

src/BitGoSDK.php

@@ -17,6 +17,8 @@
 use Composer\CaBundle\CaBundle;
 use neto737\BitGoSDK\Enum\CurrencyCode;
 use neto737\BitGoSDK\Enum\AddressType;
+use neto737\BitGoSDK\Internal\CurlTransport;
+use neto737\BitGoSDK\Internal\Transport;
 
 class BitGoSDK
 {
@@ -45,6 +47,7 @@ class BitGoSDK
     private int $connectTimeout = self::DEFAULT_CONNECT_TIMEOUT;
     private ?string $caFile = null;
     private bool $forceIPv4 = false;
+    private ?Transport $transport = null;
 
     /**
      * BitGoSDK Initialization
@@ -1021,19 +1024,14 @@ private function __execute(string $requestType = 'POST')
         $params = self::withoutNulls($this->params);
         $this->params = [];
 
-        $ch = curl_init();
-        curl_setopt_array($ch, $this->curlOptions($requestType, $params));
+        $this->transport = $this->transport ?? new CurlTransport();
+        $response = $this->transport->send($this->curlOptions($requestType, $params));
 
-        $response = curl_exec($ch);
-        $errno = curl_errno($ch);
-        $error = curl_error($ch);
-        $status = (int) curl_getinfo($ch, CURLINFO_RESPONSE_CODE);
-
-        if ($errno !== 0) {
-            return ['error' => $error !== '' ? $error : 'cURL error ' . $errno, 'status' => 0];
+        if ($response['errno'] !== 0) {
+            return ['error' => $response['error'] !== '' ? $response['error'] : 'cURL error ' . $response['errno'], 'status' => 0];
         }
 
-        return self::decodeResponse((string) $response, $status);
+        return self::decodeResponse($response['body'], $response['status']);
     }
 
     /**
@@ -1045,18 +1043,21 @@ private function __execute(string $requestType = 'POST')
     private function curlOptions(string $requestType, array $params): array
     {
         $options = [CURLOPT_URL => $this->url];
+        // Cast to an object so that no params are sent as {} rather than [].
+        $body = json_encode((object) $params);
         if ($requestType === 'GET') {
-            $options[CURLOPT_URL] = $this->url . '?' . self::buildQuery($params);
+            $query = self::buildQuery($params);
+            $options[CURLOPT_URL] = $query === '' ? $this->url : $this->url . '?' . $query;
             $options[CURLOPT_CUSTOMREQUEST] = 'GET';
         } elseif ($requestType === 'PUT') {
             $options[CURLOPT_CUSTOMREQUEST] = 'PUT';
-            $options[CURLOPT_POSTFIELDS] = json_encode($params);
+            $options[CURLOPT_POSTFIELDS] = $body;
         } elseif ($requestType === 'DELETE') {
             $options[CURLOPT_CUSTOMREQUEST] = 'DELETE';
-            $options[CURLOPT_POSTFIELDS] = json_encode($params);
+            $options[CURLOPT_POSTFIELDS] = $body;
         } elseif ($requestType === 'POST') {
             $options[CURLOPT_POST] = true;
-            $options[CURLOPT_POSTFIELDS] = json_encode($params);
+            $options[CURLOPT_POSTFIELDS] = $body;
         }
         $options[CURLOPT_SSL_VERIFYPEER] = true;
         $options[CURLOPT_RETURNTRANSFER] = true;

src/Internal/CurlTransport.php

@@ -0,0 +1,41 @@
+<?php
+
+/**
+ *  ____  _ _    ____      ____  ____  _  __
+ * | __ )(_) |_ / ___| ___/ ___||  _ \| |/ /
+ * |  _ \| | __| |  _ / _ \___ \| | | | ' /
+ * | |_) | | |_| |_| | (_) |__) | |_| | . \
+ * |____/|_|\__|\____|\___/____/|____/|_|\_\
+ *
+ * @package BitGoSDK PHP
+ * @author  Neto Melo <neto737@live.com>
+ * @license https://opensource.org/licenses/MIT MIT License
+ */
+
+namespace neto737\BitGoSDK\Internal;
+
+/**
+ * Sends requests with ext-curl.
+ *
+ * @internal Not part of the public API. It may change or go away in any release.
+ */
+final class CurlTransport implements Transport
+{
+    public function send(array $options): array
+    {
+        $ch = curl_init();
+        if ($ch === false) {
+            return ['body' => '', 'status' => 0, 'errno' => CURLE_FAILED_INIT, 'error' => 'Failed to initialise cURL'];
+        }
+
+        curl_setopt_array($ch, $options);
+        $body = curl_exec($ch);
+
+        return [
+            'body' => is_string($body) ? $body : '',
+            'status' => (int) curl_getinfo($ch, CURLINFO_RESPONSE_CODE),
+            'errno' => curl_errno($ch),
+            'error' => curl_error($ch),
+        ];
+    }
+}

src/Internal/Transport.php

@@ -0,0 +1,30 @@
+<?php
+
+/**
+ *  ____  _ _    ____      ____  ____  _  __
+ * | __ )(_) |_ / ___| ___/ ___||  _ \| |/ /
+ * |  _ \| | __| |  _ / _ \___ \| | | | ' /
+ * | |_) | | |_| |_| | (_) |__) | |_| | . \
+ * |____/|_|\__|\____|\___/____/|____/|_|\_\
+ *
+ * @package BitGoSDK PHP
+ * @author  Neto Melo <neto737@live.com>
+ * @license https://opensource.org/licenses/MIT MIT License
+ */
+
+namespace neto737\BitGoSDK\Internal;
+
+/**
+ * Sends one HTTP request described by cURL options. Lets tests capture requests instead of sending them.
+ *
+ * @internal Not part of the public API. It may change or go away in any release.
+ */
+interface Transport
+{
+    /**
+     * @param array<int, mixed> $options    cURL options, as passed to curl_setopt_array()
+     * @return array{body: string, status: int, errno: int, error: string}
+     *                                      The response body and HTTP status, or a non-zero cURL errno and its message
+     */
+    public function send(array $options): array;
+}

tests/AccessesPrivateMembers.php

@@ -6,7 +6,7 @@
 use ReflectionProperty;
 
 /**
- * Reflection helpers for reaching client internals until requests go through a capturable transport.
+ * Reflection helpers for reaching client internals. To check what a request sends, prefer Fixtures\RecordingTransport.
  */
 trait AccessesPrivateMembers
 {

tests/CurlTransportTest.php

@@ -0,0 +1,78 @@
+<?php
+
+namespace neto737\BitGoSDK\Tests;
+
+use neto737\BitGoSDK\BitGoExpress;
+use neto737\BitGoSDK\BitGoSDK;
+use neto737\BitGoSDK\Internal\CurlTransport;
+use PHPUnit\Framework\TestCase;
+
+/**
+ * The real cURL transport, exercised without reaching the network.
+ */
+final class CurlTransportTest extends TestCase
+{
+    use AccessesPrivateMembers;
+
+    public function testReturnsTheBody(): void
+    {
+        $file = tempnam(sys_get_temp_dir(), 'bitgo');
+        self::assertIsString($file);
+        file_put_contents($file, '{"id":"w1"}');
+
+        try {
+            $response = (new CurlTransport())->send([
+                CURLOPT_URL => 'file://' . str_replace('\\', '/', (string) realpath($file)),
+                CURLOPT_RETURNTRANSFER => true,
+            ]);
+        } finally {
+            unlink($file);
+        }
+
+        self::assertSame('{"id":"w1"}', $response['body']);
+        self::assertSame(0, $response['errno']);
+        self::assertSame('', $response['error']);
+    }
+
+    public function testReportsCurlErrors(): void
+    {
+        $response = (new CurlTransport())->send([
+            CURLOPT_URL => 'nosuchscheme://example.com/',
+            CURLOPT_RETURNTRANSFER => true,
+        ]);
+
+        self::assertSame('', $response['body']);
+        self::assertSame(0, $response['status']);
+        self::assertSame(CURLE_UNSUPPORTED_PROTOCOL, $response['errno']);
+        self::assertNotSame('', $response['error']);
+    }
+
+    /**
+     * @dataProvider clients
+     * @param BitGoSDK|BitGoExpress $client
+     * @param callable(mixed): mixed $request
+     */
+    public function testClientsSendThroughCurlByDefault($client, callable $request): void
+    {
+        // cURL rejects the scheme before anything leaves the machine.
+        $this->setPrivate($client, 'APIEndpoint', 'nosuchscheme://example.com/api/v2/tbtc');
+
+        $result = (array) $request($client);
+
+        self::assertInstanceOf(CurlTransport::class, $this->getPrivate($client, 'transport'));
+        self::assertSame(0, $result['status']);
+        self::assertIsString($result['error']);
+        self::assertNotSame('', $result['error']);
+    }
+
+    /**
+     * @return array<string, array{BitGoSDK|BitGoExpress, callable(mixed): mixed}>
+     */
+    public static function clients(): array
+    {
+        return [
+            'BitGoSDK' => [new BitGoSDK('token', 'tbtc', true), static fn (BitGoSDK $c) => $c->getWallet()],
+            'BitGoExpress' => [new BitGoExpress('localhost', 3080, 'tbtc'), static fn (BitGoExpress $c) => $c->buildTransaction([])],
+        ];
+    }
+}

tests/DeprecatedAliasTest.php

@@ -4,6 +4,7 @@
 
 use neto737\BitGoSDK\BitGoExpress;
 use neto737\BitGoSDK\BitGoSDK;
+use neto737\BitGoSDK\Tests\Fixtures\RecordingTransport;
 use neto737\BitGoSDK\Tests\Fixtures\RecordingWebhookSdk;
 use PHPUnit\Framework\TestCase;
 use ReflectionMethod;
@@ -13,16 +14,15 @@
  */
 final class DeprecatedAliasTest extends TestCase
 {
-    use AccessesPrivateMembers;
-
     public function testFanoutBuildsTheFanoutUrl(): void
     {
-        $express = new BitGoExpress('127.0.0.1', 1, 'tbtc');
+        $express = new BitGoExpress('localhost', 3080, 'tbtc');
         $express->walletId = 'wallet-id';
+        $transport = RecordingTransport::on($express);
 
         $express->fanoutWalletUnspents('passphrase');
 
-        self::assertSame('http://127.0.0.1:1/api/v2/tbtc/wallet/wallet-id/fanoutunspents', $this->getPrivate($express, 'url'));
+        self::assertSame('http://localhost:3080/api/v2/tbtc/wallet/wallet-id/fanoutunspents', $transport->last()->url);
     }
 
     public function testFunoutForwardsEveryArgumentToFanout(): void