[CodeHealth] Spanifying wallet key signing

This is a general rework away from `const std::vector<uint8_t>&` when passing bytes around to using `span` instances, which are more generic. It is also much preferred that with `span` we can pass the data around by value, rather than by reference.
brave · Oct 7, 2024 · 43eb3da · 43eb3da
1 parent 74b1dfa
commit 43eb3da
Show file tree

Hide file tree

Showing 17 changed files with 83 additions and 76 deletions.
diff --git a/components/brave_wallet/browser/bitcoin/bitcoin_hd_keyring.cc b/components/brave_wallet/browser/bitcoin/bitcoin_hd_keyring.cc
@@ -10,6 +10,7 @@
 #include <utility>
 
 #include "base/check.h"
+#include "base/containers/span.h"
 #include "base/notimplemented.h"
 #include "base/notreached.h"
 #include "brave/components/brave_wallet/common/bitcoin_utils.h"
@@ -60,7 +61,7 @@ std::optional<std::vector<uint8_t>> BitcoinHDKeyring::SignMessage(
 }
 
 std::string BitcoinHDKeyring::ImportAccount(
-    const std::vector<uint8_t>& private_key) {
+    base::span<const uint8_t> private_key) {
   NOTREACHED_IN_MIGRATION();
   return "";
 }

diff --git a/components/brave_wallet/browser/bitcoin/bitcoin_hd_keyring.h b/components/brave_wallet/browser/bitcoin/bitcoin_hd_keyring.h
@@ -11,6 +11,7 @@
 #include <string>
 #include <vector>
 
+#include "base/containers/span.h"
 #include "brave/components/brave_wallet/browser/bitcoin/bitcoin_base_keyring.h"
 #include "brave/components/brave_wallet/browser/secp256k1_hd_keyring.h"
 #include "brave/components/brave_wallet/common/brave_wallet.mojom.h"
@@ -36,7 +37,7 @@ class BitcoinHDKeyring : public BitcoinBaseKeyring, public Secp256k1HDKeyring {
       const mojom::BitcoinKeyId& key_id,
       base::span<const uint8_t, 32> message) override;
 
-  std::string ImportAccount(const std::vector<uint8_t>& private_key) override;
+  std::string ImportAccount(base::span<const uint8_t> private_key) override;
   bool RemoveImportedAccount(const std::string& address) override;
   std::string GetDiscoveryAddress(size_t index) const override;
   std::vector<std::string> GetImportedAccountsForTesting() const override;

diff --git a/components/brave_wallet/browser/ethereum_keyring.cc b/components/brave_wallet/browser/ethereum_keyring.cc
@@ -8,6 +8,7 @@
 #include <optional>
 
 #include "base/base64.h"
+#include "base/containers/span.h"
 #include "base/strings/string_number_conversions.h"
 #include "brave/components/brave_wallet/browser/eth_transaction.h"
 #include "brave/components/brave_wallet/common/eth_address.h"
@@ -18,7 +19,7 @@ namespace brave_wallet {
 namespace {
 
 // Get the 32 byte message hash
-std::vector<uint8_t> GetMessageHash(const std::vector<uint8_t>& message) {
+std::vector<uint8_t> GetMessageHash(base::span<const uint8_t> message) {
   std::string prefix("\x19");
   prefix += std::string("Ethereum Signed Message:\n" +
                         base::NumberToString(message.size()));
@@ -33,20 +34,19 @@ EthereumKeyring::EthereumKeyring(base::span<const uint8_t> seed)
     : Secp256k1HDKeyring(seed, GetRootPath(mojom::KeyringId::kDefault)) {}
 
 // static
-bool EthereumKeyring::RecoverAddress(const std::vector<uint8_t>& message,
-                                     const std::vector<uint8_t>& signature,
-                                     std::string* address) {
-  CHECK(address);
+std::optional<std::string> EthereumKeyring::RecoverAddress(
+    base::span<const uint8_t> message,
+    base::span<const uint8_t> signature) {
   // A compact ECDSA signature (recovery id byte + 64 bytes).
   if (signature.size() != kCompactSignatureSize + 1) {
-    return false;
+    return std::nullopt;
   }
 
-  std::vector<uint8_t> signature_only = signature;
+  std::vector<uint8_t> signature_only(signature.begin(), signature.end());
   uint8_t v = signature_only.back();
   if (v < 27) {
     VLOG(1) << "v should be >= 27";
-    return false;
+    return std::nullopt;
   }
 
   // v = chain_id ? recid + chain_id * 2 + 35 : recid + 27;
@@ -64,24 +64,23 @@ bool EthereumKeyring::RecoverAddress(const std::vector<uint8_t>& message,
       key.RecoverCompact(false, hash, signature_only, recid);
   if (public_key.size() != 65) {
     VLOG(1) << "public key should be 65 bytes";
-    return false;
+    return std::nullopt;
   }
 
   uint8_t first_byte = *public_key.begin();
   public_key.erase(public_key.begin());
   if (first_byte != 4) {
     VLOG(1) << "First byte of public key should be 4";
-    return false;
+    return std::nullopt;
   }
 
   EthAddress addr = EthAddress::FromPublicKey(public_key);
-  *address = addr.ToChecksumAddress();
-  return true;
+  return addr.ToChecksumAddress();
 }
 
 std::vector<uint8_t> EthereumKeyring::SignMessage(
     const std::string& address,
-    const std::vector<uint8_t>& message,
+    base::span<const uint8_t> message,
     uint256_t chain_id,
     bool is_eip712) {
   HDKey* hd_key = GetHDKeyFromAddress(address);
@@ -98,7 +97,7 @@ std::vector<uint8_t> EthereumKeyring::SignMessage(
       return std::vector<uint8_t>();
     }
 
-    hash = message;
+    hash.assign(message.begin(), message.end());
   }
 
   int recid;
@@ -154,9 +153,9 @@ bool EthereumKeyring::GetPublicKeyFromX25519_XSalsa20_Poly1305(
 std::optional<std::vector<uint8_t>>
 EthereumKeyring::DecryptCipherFromX25519_XSalsa20_Poly1305(
     const std::string& version,
-    const std::vector<uint8_t>& nonce,
-    const std::vector<uint8_t>& ephemeral_public_key,
-    const std::vector<uint8_t>& ciphertext,
+    base::span<const uint8_t> nonce,
+    base::span<const uint8_t> ephemeral_public_key,
+    base::span<const uint8_t> ciphertext,
     const std::string& address) {
   HDKey* hd_key = GetHDKeyFromAddress(address);
   if (!hd_key) {

diff --git a/components/brave_wallet/browser/ethereum_keyring.h b/components/brave_wallet/browser/ethereum_keyring.h
@@ -11,6 +11,7 @@
 #include <string>
 #include <vector>
 
+#include "base/containers/span.h"
 #include "brave/components/brave_wallet/browser/secp256k1_hd_keyring.h"
 #include "brave/components/brave_wallet/common/brave_wallet_types.h"
 
@@ -29,12 +30,12 @@ class EthereumKeyring : public Secp256k1HDKeyring {
   // message: The keccak256 hash of the message (33 bytes = 04 prefix + 32
   // bytes)
   // signature: The 64 byte signature + v parameter (0 chain id assumed)
-  static bool RecoverAddress(const std::vector<uint8_t>& message,
-                             const std::vector<uint8_t>& signature,
-                             std::string* address);
+  static std::optional<std::string> RecoverAddress(
+      base::span<const uint8_t> message,
+      base::span<const uint8_t> signature);
 
   std::vector<uint8_t> SignMessage(const std::string& address,
-                                   const std::vector<uint8_t>& message,
+                                   base::span<const uint8_t> message,
                                    uint256_t chain_id,
                                    bool is_eip712);
 
@@ -46,9 +47,9 @@ class EthereumKeyring : public Secp256k1HDKeyring {
                                                 std::string* key);
   std::optional<std::vector<uint8_t>> DecryptCipherFromX25519_XSalsa20_Poly1305(
       const std::string& version,
-      const std::vector<uint8_t>& nonce,
-      const std::vector<uint8_t>& ephemeral_public_key,
-      const std::vector<uint8_t>& ciphertext,
+      base::span<const uint8_t> nonce,
+      base::span<const uint8_t> ephemeral_public_key,
+      base::span<const uint8_t> ciphertext,
       const std::string& address);
 
   std::string EncodePrivateKeyForExport(const std::string& address) override;

diff --git a/components/brave_wallet/browser/ethereum_provider_impl.cc b/components/brave_wallet/browser/ethereum_provider_impl.cc
@@ -479,19 +479,19 @@ void EthereumProviderImpl::RecoverAddress(const std::string& message,
     return RejectInvalidParams(std::move(id), std::move(callback));
   }
 
-  std::vector<uint8_t> message_bytes;
-  if (!PrefixedHexStringToBytes(message, &message_bytes)) {
+  auto message_bytes = PrefixedHexStringToBytes(message);
+  if (!message_bytes) {
     return RejectInvalidParams(std::move(id), std::move(callback));
   }
 
-  std::vector<uint8_t> signature_bytes;
-  if (!PrefixedHexStringToBytes(signature, &signature_bytes)) {
+  auto signature_bytes = PrefixedHexStringToBytes(signature);
+  if (!signature_bytes) {
     return RejectInvalidParams(std::move(id), std::move(callback));
   }
 
-  std::string address;
-  if (!keyring_service_->RecoverAddressByDefaultKeyring(
-          message_bytes, signature_bytes, &address)) {
+  auto address = keyring_service_->RecoverAddressByDefaultKeyring(
+      *message_bytes, *signature_bytes);
+  if (!address) {
     base::Value formed_response = GetProviderErrorDictionary(
         mojom::ProviderError::kInternalError,
         l10n_util::GetStringUTF8(IDS_WALLET_INTERNAL_ERROR));
@@ -502,7 +502,8 @@ void EthereumProviderImpl::RecoverAddress(const std::string& message,
   }
 
   reject = false;
-  std::move(callback).Run(std::move(id), base::Value(address), reject, "",
+  std::move(callback).Run(std::move(id),
+                          base::Value(std::move(address).value()), reject, "",
                           false);
 }
 
@@ -669,8 +670,8 @@ void EthereumProviderImpl::ContinueDecryptWithSanitizedJson(
 void EthereumProviderImpl::SignTypedMessage(
     const std::string& address,
     const std::string& message,
-    const std::vector<uint8_t>& domain_hash,
-    const std::vector<uint8_t>& primary_hash,
+    base::span<const uint8_t> domain_hash,
+    base::span<const uint8_t> primary_hash,
     mojom::EthSignTypedDataMetaPtr meta,
     base::Value::Dict domain,
     RequestCallback callback,

diff --git a/components/brave_wallet/browser/ethereum_provider_impl.h b/components/brave_wallet/browser/ethereum_provider_impl.h
@@ -103,8 +103,8 @@ class EthereumProviderImpl final : public mojom::EthereumProvider,
   // domain is the domain separator defined in eip712
   void SignTypedMessage(const std::string& address,
                         const std::string& message,
-                        const std::vector<uint8_t>& domain_hash,
-                        const std::vector<uint8_t>& primary_hash,
+                        base::span<const uint8_t> domain_hash,
+                        base::span<const uint8_t> primary_hash,
                         mojom::EthSignTypedDataMetaPtr meta,
                         base::Value::Dict domain,
                         RequestCallback callback,

diff --git a/components/brave_wallet/browser/hd_keyring.h b/components/brave_wallet/browser/hd_keyring.h
@@ -10,6 +10,7 @@
 #include <string>
 #include <vector>
 
+#include "base/containers/span.h"
 #include "brave/components/brave_wallet/common/brave_wallet.mojom.h"
 
 namespace brave_wallet {
@@ -34,8 +35,7 @@ class HDKeyring {
   // TODO(apaymyshev): imported accounts should be handled by separate keyring
   // class.
   // address will be returned
-  virtual std::string ImportAccount(
-      const std::vector<uint8_t>& private_key) = 0;
+  virtual std::string ImportAccount(base::span<const uint8_t> private_key) = 0;
   virtual bool RemoveImportedAccount(const std::string& address) = 0;
 
   virtual std::string GetDiscoveryAddress(size_t index) const = 0;

diff --git a/components/brave_wallet/browser/internal/hd_key.cc b/components/brave_wallet/browser/internal/hd_key.cc
@@ -62,7 +62,7 @@ const secp256k1_context* GetSecp256k1Ctx() {
 }
 
 bool UTCPasswordVerification(const std::string& derived_key,
-                             const std::vector<uint8_t>& ciphertext,
+                             base::span<const uint8_t> ciphertext,
                              const std::string& mac,
                              size_t dklen) {
   std::vector<uint8_t> mac_verification_input(derived_key.end() - dklen / 2,
@@ -79,8 +79,8 @@ bool UTCPasswordVerification(const std::string& derived_key,
 }
 
 bool UTCDecryptPrivateKey(const std::string& derived_key,
-                          const std::vector<uint8_t>& ciphertext,
-                          const std::vector<uint8_t>& iv,
+                          base::span<const uint8_t> ciphertext,
+                          base::span<const uint8_t> iv,
                           std::vector<uint8_t>* private_key,
                           size_t dklen) {
   if (!private_key) {
@@ -198,7 +198,7 @@ std::unique_ptr<HDKey::ParsedExtendedKey> HDKey::GenerateFromExtendedKey(
 
 // static
 std::unique_ptr<HDKey> HDKey::GenerateFromPrivateKey(
-    const std::vector<uint8_t>& private_key) {
+    base::span<const uint8_t> private_key) {
   if (private_key.size() != 32) {
     return nullptr;
   }

diff --git a/components/brave_wallet/browser/internal/hd_key.h b/components/brave_wallet/browser/internal/hd_key.h
@@ -59,7 +59,7 @@ class HDKey {
   static std::unique_ptr<ParsedExtendedKey> GenerateFromExtendedKey(
       const std::string& key);
   static std::unique_ptr<HDKey> GenerateFromPrivateKey(
-      const std::vector<uint8_t>& private_key);
+      base::span<const uint8_t> private_key);
   // https://github.com/ethereum/wiki/wiki/Web3-Secret-Storage-Definition
   static std::unique_ptr<HDKey> GenerateFromV3UTC(const std::string& password,
                                                   const std::string& json);

diff --git a/components/brave_wallet/browser/keyring_service.cc b/components/brave_wallet/browser/keyring_service.cc
@@ -1333,7 +1333,7 @@ mojom::AccountInfoPtr KeyringService::ImportAccountForKeyring(
     mojom::CoinType coin,
     mojom::KeyringId keyring_id,
     const std::string& account_name,
-    const std::vector<uint8_t>& private_key) {
+    base::span<const uint8_t> private_key) {
   auto* keyring = GetHDKeyringById(keyring_id);
   if (!keyring) {
     return {};
@@ -1607,7 +1607,7 @@ KeyringService::SignatureWithError::~SignatureWithError() = default;
 
 KeyringService::SignatureWithError KeyringService::SignMessageByDefaultKeyring(
     const mojom::AccountIdPtr& account_id,
-    const std::vector<uint8_t>& message,
+    base::span<const uint8_t> message,
     bool is_eip712) {
   CHECK(account_id);
   SignatureWithError ret;
@@ -1634,12 +1634,10 @@ KeyringService::SignatureWithError KeyringService::SignMessageByDefaultKeyring(
   return ret;
 }
 
-bool KeyringService::RecoverAddressByDefaultKeyring(
-    const std::vector<uint8_t>& message,
-    const std::vector<uint8_t>& signature,
-    std::string* address) {
-  CHECK(address);
-  return EthereumKeyring::RecoverAddress(message, signature, address);
+std::optional<std::string> KeyringService::RecoverAddressByDefaultKeyring(
+    base::span<const uint8_t> message,
+    base::span<const uint8_t> signature) {
+  return EthereumKeyring::RecoverAddress(message, signature);
 }
 
 bool KeyringService::GetPublicKeyFromX25519_XSalsa20_Poly1305ByDefaultKeyring(
@@ -1659,9 +1657,9 @@ std::optional<std::vector<uint8_t>>
 KeyringService::DecryptCipherFromX25519_XSalsa20_Poly1305ByDefaultKeyring(
     const mojom::AccountIdPtr& account_id,
     const std::string& version,
-    const std::vector<uint8_t>& nonce,
-    const std::vector<uint8_t>& ephemeral_public_key,
-    const std::vector<uint8_t>& ciphertext) {
+    base::span<const uint8_t> nonce,
+    base::span<const uint8_t> ephemeral_public_key,
+    base::span<const uint8_t> ciphertext) {
   CHECK(account_id);
   if (account_id->keyring_id != mojom::kDefaultKeyringId) {
     return std::nullopt;
@@ -1677,7 +1675,7 @@ KeyringService::DecryptCipherFromX25519_XSalsa20_Poly1305ByDefaultKeyring(
 
 std::vector<uint8_t> KeyringService::SignMessageBySolanaKeyring(
     const mojom::AccountIdPtr& account_id,
-    const std::vector<uint8_t>& message) {
+    base::span<const uint8_t> message) {
   if (account_id->keyring_id != mojom::KeyringId::kSolana) {
     return {};
   }

diff --git a/components/brave_wallet/browser/keyring_service.h b/components/brave_wallet/browser/keyring_service.h
@@ -150,25 +150,25 @@ class KeyringService : public mojom::KeyringService {
   };
   SignatureWithError SignMessageByDefaultKeyring(
       const mojom::AccountIdPtr& account_id,
-      const std::vector<uint8_t>& message,
+      base::span<const uint8_t> message,
       bool is_eip712 = false);
 
   std::vector<uint8_t> SignMessageBySolanaKeyring(
       const mojom::AccountIdPtr& account_id,
-      const std::vector<uint8_t>& message);
-  bool RecoverAddressByDefaultKeyring(const std::vector<uint8_t>& message,
-                                      const std::vector<uint8_t>& signature,
-                                      std::string* address);
+      base::span<const uint8_t> message);
+  std::optional<std::string> RecoverAddressByDefaultKeyring(
+      base::span<const uint8_t> message,
+      base::span<const uint8_t> signature);
   bool GetPublicKeyFromX25519_XSalsa20_Poly1305ByDefaultKeyring(
       const mojom::AccountIdPtr& account_id,
       std::string* key);
   std::optional<std::vector<uint8_t>>
   DecryptCipherFromX25519_XSalsa20_Poly1305ByDefaultKeyring(
       const mojom::AccountIdPtr& account_id,
       const std::string& version,
-      const std::vector<uint8_t>& nonce,
-      const std::vector<uint8_t>& ephemeral_public_key,
-      const std::vector<uint8_t>& ciphertext);
+      base::span<const uint8_t> nonce,
+      base::span<const uint8_t> ephemeral_public_key,
+      base::span<const uint8_t> ciphertext);
 
   void AddAccountsWithDefaultName(const mojom::CoinType& coin_type,
                                   mojom::KeyringId keyring_id,
@@ -348,7 +348,7 @@ class KeyringService : public mojom::KeyringService {
       mojom::CoinType coin,
       mojom::KeyringId keyring_id,
       const std::string& account_name,
-      const std::vector<uint8_t>& private_key);
+      base::span<const uint8_t> private_key);
 
   std::vector<mojom::AccountInfoPtr> GetAccountInfosForKeyring(
       mojom::KeyringId keyring_id) const;

diff --git a/components/brave_wallet/browser/secp256k1_hd_keyring.cc b/components/brave_wallet/browser/secp256k1_hd_keyring.cc
@@ -77,7 +77,7 @@ void Secp256k1HDKeyring::RemoveLastHDAccount() {
 }
 
 std::string Secp256k1HDKeyring::ImportAccount(
-    const std::vector<uint8_t>& private_key) {
+    base::span<const uint8_t> private_key) {
   std::unique_ptr<HDKey> hd_key = HDKey::GenerateFromPrivateKey(private_key);
   if (!hd_key) {
     return std::string();