From 43eb3da39bf24fc6a38242ac2876dc26c3d58b1c Mon Sep 17 00:00:00 2001 From: Claudio DeSouza Date: Mon, 7 Oct 2024 02:06:29 +0100 Subject: [PATCH] [CodeHealth] Spanifying wallet key signing This is a general rework away from `const std::vector&` 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. --- .../browser/bitcoin/bitcoin_hd_keyring.cc | 3 +- .../browser/bitcoin/bitcoin_hd_keyring.h | 3 +- .../brave_wallet/browser/ethereum_keyring.cc | 33 +++++++++---------- .../brave_wallet/browser/ethereum_keyring.h | 15 +++++---- .../browser/ethereum_provider_impl.cc | 21 ++++++------ .../browser/ethereum_provider_impl.h | 4 +-- components/brave_wallet/browser/hd_keyring.h | 4 +-- .../brave_wallet/browser/internal/hd_key.cc | 8 ++--- .../brave_wallet/browser/internal/hd_key.h | 2 +- .../brave_wallet/browser/keyring_service.cc | 22 ++++++------- .../brave_wallet/browser/keyring_service.h | 18 +++++----- .../browser/secp256k1_hd_keyring.cc | 2 +- .../browser/secp256k1_hd_keyring.h | 3 +- .../brave_wallet/browser/solana_keyring.cc | 5 +-- .../brave_wallet/browser/solana_keyring.h | 6 ++-- .../common/eth_sign_typed_data_helper.cc | 5 +-- .../common/eth_sign_typed_data_helper.h | 5 +-- 17 files changed, 83 insertions(+), 76 deletions(-) diff --git a/components/brave_wallet/browser/bitcoin/bitcoin_hd_keyring.cc b/components/brave_wallet/browser/bitcoin/bitcoin_hd_keyring.cc index 7f5d2faddc8c..1981146d1cdc 100644 --- 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 #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> BitcoinHDKeyring::SignMessage( } std::string BitcoinHDKeyring::ImportAccount( - const std::vector& private_key) { + base::span 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 index cdb10978bea4..5b5af22b52a2 100644 --- 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 #include +#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 message) override; - std::string ImportAccount(const std::vector& private_key) override; + std::string ImportAccount(base::span private_key) override; bool RemoveImportedAccount(const std::string& address) override; std::string GetDiscoveryAddress(size_t index) const override; std::vector GetImportedAccountsForTesting() const override; diff --git a/components/brave_wallet/browser/ethereum_keyring.cc b/components/brave_wallet/browser/ethereum_keyring.cc index 208ff7893d60..def6cd757fb0 100644 --- a/components/brave_wallet/browser/ethereum_keyring.cc +++ b/components/brave_wallet/browser/ethereum_keyring.cc @@ -8,6 +8,7 @@ #include #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 GetMessageHash(const std::vector& message) { +std::vector GetMessageHash(base::span message) { std::string prefix("\x19"); prefix += std::string("Ethereum Signed Message:\n" + base::NumberToString(message.size())); @@ -33,20 +34,19 @@ EthereumKeyring::EthereumKeyring(base::span seed) : Secp256k1HDKeyring(seed, GetRootPath(mojom::KeyringId::kDefault)) {} // static -bool EthereumKeyring::RecoverAddress(const std::vector& message, - const std::vector& signature, - std::string* address) { - CHECK(address); +std::optional EthereumKeyring::RecoverAddress( + base::span message, + base::span signature) { // A compact ECDSA signature (recovery id byte + 64 bytes). if (signature.size() != kCompactSignatureSize + 1) { - return false; + return std::nullopt; } - std::vector signature_only = signature; + std::vector 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& 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 EthereumKeyring::SignMessage( const std::string& address, - const std::vector& message, + base::span message, uint256_t chain_id, bool is_eip712) { HDKey* hd_key = GetHDKeyFromAddress(address); @@ -98,7 +97,7 @@ std::vector EthereumKeyring::SignMessage( return std::vector(); } - hash = message; + hash.assign(message.begin(), message.end()); } int recid; @@ -154,9 +153,9 @@ bool EthereumKeyring::GetPublicKeyFromX25519_XSalsa20_Poly1305( std::optional> EthereumKeyring::DecryptCipherFromX25519_XSalsa20_Poly1305( const std::string& version, - const std::vector& nonce, - const std::vector& ephemeral_public_key, - const std::vector& ciphertext, + base::span nonce, + base::span ephemeral_public_key, + base::span 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 index ba38bfbffae0..a094a44b1a4b 100644 --- a/components/brave_wallet/browser/ethereum_keyring.h +++ b/components/brave_wallet/browser/ethereum_keyring.h @@ -11,6 +11,7 @@ #include #include +#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& message, - const std::vector& signature, - std::string* address); + static std::optional RecoverAddress( + base::span message, + base::span signature); std::vector SignMessage(const std::string& address, - const std::vector& message, + base::span message, uint256_t chain_id, bool is_eip712); @@ -46,9 +47,9 @@ class EthereumKeyring : public Secp256k1HDKeyring { std::string* key); std::optional> DecryptCipherFromX25519_XSalsa20_Poly1305( const std::string& version, - const std::vector& nonce, - const std::vector& ephemeral_public_key, - const std::vector& ciphertext, + base::span nonce, + base::span ephemeral_public_key, + base::span 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 index 07f5954f0aa7..5be0872de48b 100644 --- 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 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 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& domain_hash, - const std::vector& primary_hash, + base::span domain_hash, + base::span 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 index f71e515b95c0..6d3b548ac05d 100644 --- 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& domain_hash, - const std::vector& primary_hash, + base::span domain_hash, + base::span 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 index 9e78ad2cd9df..c7a1bcc05c8e 100644 --- a/components/brave_wallet/browser/hd_keyring.h +++ b/components/brave_wallet/browser/hd_keyring.h @@ -10,6 +10,7 @@ #include #include +#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& private_key) = 0; + virtual std::string ImportAccount(base::span 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 index d68e6489514e..b7b7b65e6249 100644 --- 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& ciphertext, + base::span ciphertext, const std::string& mac, size_t dklen) { std::vector 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& ciphertext, - const std::vector& iv, + base::span ciphertext, + base::span iv, std::vector* private_key, size_t dklen) { if (!private_key) { @@ -198,7 +198,7 @@ std::unique_ptr HDKey::GenerateFromExtendedKey( // static std::unique_ptr HDKey::GenerateFromPrivateKey( - const std::vector& private_key) { + base::span 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 index ddbea097eddd..47013a4c9a34 100644 --- 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 GenerateFromExtendedKey( const std::string& key); static std::unique_ptr GenerateFromPrivateKey( - const std::vector& private_key); + base::span private_key); // https://github.com/ethereum/wiki/wiki/Web3-Secret-Storage-Definition static std::unique_ptr 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 index bcf6d0917005..13b143ca4688 100644 --- 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& private_key) { + base::span 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& message, + base::span 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& message, - const std::vector& signature, - std::string* address) { - CHECK(address); - return EthereumKeyring::RecoverAddress(message, signature, address); +std::optional KeyringService::RecoverAddressByDefaultKeyring( + base::span message, + base::span signature) { + return EthereumKeyring::RecoverAddress(message, signature); } bool KeyringService::GetPublicKeyFromX25519_XSalsa20_Poly1305ByDefaultKeyring( @@ -1659,9 +1657,9 @@ std::optional> KeyringService::DecryptCipherFromX25519_XSalsa20_Poly1305ByDefaultKeyring( const mojom::AccountIdPtr& account_id, const std::string& version, - const std::vector& nonce, - const std::vector& ephemeral_public_key, - const std::vector& ciphertext) { + base::span nonce, + base::span ephemeral_public_key, + base::span ciphertext) { CHECK(account_id); if (account_id->keyring_id != mojom::kDefaultKeyringId) { return std::nullopt; @@ -1677,7 +1675,7 @@ KeyringService::DecryptCipherFromX25519_XSalsa20_Poly1305ByDefaultKeyring( std::vector KeyringService::SignMessageBySolanaKeyring( const mojom::AccountIdPtr& account_id, - const std::vector& message) { + base::span 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 index 9b2657c6c95b..e2bccb83ee8e 100644 --- a/components/brave_wallet/browser/keyring_service.h +++ b/components/brave_wallet/browser/keyring_service.h @@ -150,15 +150,15 @@ class KeyringService : public mojom::KeyringService { }; SignatureWithError SignMessageByDefaultKeyring( const mojom::AccountIdPtr& account_id, - const std::vector& message, + base::span message, bool is_eip712 = false); std::vector SignMessageBySolanaKeyring( const mojom::AccountIdPtr& account_id, - const std::vector& message); - bool RecoverAddressByDefaultKeyring(const std::vector& message, - const std::vector& signature, - std::string* address); + base::span message); + std::optional RecoverAddressByDefaultKeyring( + base::span message, + base::span signature); bool GetPublicKeyFromX25519_XSalsa20_Poly1305ByDefaultKeyring( const mojom::AccountIdPtr& account_id, std::string* key); @@ -166,9 +166,9 @@ class KeyringService : public mojom::KeyringService { DecryptCipherFromX25519_XSalsa20_Poly1305ByDefaultKeyring( const mojom::AccountIdPtr& account_id, const std::string& version, - const std::vector& nonce, - const std::vector& ephemeral_public_key, - const std::vector& ciphertext); + base::span nonce, + base::span ephemeral_public_key, + base::span 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& private_key); + base::span private_key); std::vector 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 index 4038326c33bd..2d79ad6aff59 100644 --- 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& private_key) { + base::span private_key) { std::unique_ptr hd_key = HDKey::GenerateFromPrivateKey(private_key); if (!hd_key) { return std::string(); diff --git a/components/brave_wallet/browser/secp256k1_hd_keyring.h b/components/brave_wallet/browser/secp256k1_hd_keyring.h index fcd14018c6cc..fe7a280f461a 100644 --- a/components/brave_wallet/browser/secp256k1_hd_keyring.h +++ b/components/brave_wallet/browser/secp256k1_hd_keyring.h @@ -11,6 +11,7 @@ #include #include "base/containers/flat_map.h" +#include "base/containers/span.h" #include "base/gtest_prod_util.h" #include "brave/components/brave_wallet/browser/hd_keyring.h" #include "brave/components/brave_wallet/browser/internal/hd_key.h" @@ -32,7 +33,7 @@ class Secp256k1HDKeyring : public HDKeyring { std::optional AddNewHDAccount() override; void RemoveLastHDAccount() override; - std::string ImportAccount(const std::vector& private_key) override; + std::string ImportAccount(base::span private_key) override; bool RemoveImportedAccount(const std::string& address) override; std::string GetDiscoveryAddress(size_t index) const override; diff --git a/components/brave_wallet/browser/solana_keyring.cc b/components/brave_wallet/browser/solana_keyring.cc index 9bce5e63f8be..9e80cbd9715d 100644 --- a/components/brave_wallet/browser/solana_keyring.cc +++ b/components/brave_wallet/browser/solana_keyring.cc @@ -10,6 +10,7 @@ #include #include "base/containers/contains.h" +#include "base/containers/span.h" #include "base/ranges/algorithm.h" #include "brave/components/brave_wallet/browser/brave_wallet_utils.h" #include "brave/components/brave_wallet/common/brave_wallet.mojom.h" @@ -64,7 +65,7 @@ void SolanaKeyring::RemoveLastHDAccount() { accounts_.pop_back(); } -std::string SolanaKeyring::ImportAccount(const std::vector& keypair) { +std::string SolanaKeyring::ImportAccount(base::span keypair) { // extract private key from keypair std::vector private_key = std::vector( keypair.begin(), keypair.begin() + kSolanaPrikeySize); @@ -106,7 +107,7 @@ std::string SolanaKeyring::EncodePrivateKeyForExport( std::vector SolanaKeyring::SignMessage( const std::string& address, - const std::vector& message) { + base::span message) { HDKeyEd25519* hd_key = static_cast(GetHDKeyFromAddress(address)); if (!hd_key) { diff --git a/components/brave_wallet/browser/solana_keyring.h b/components/brave_wallet/browser/solana_keyring.h index 8918885bff48..519679275706 100644 --- a/components/brave_wallet/browser/solana_keyring.h +++ b/components/brave_wallet/browser/solana_keyring.h @@ -11,6 +11,8 @@ #include #include +#include "base/containers/flat_map.h" +#include "base/containers/span.h" #include "brave/components/brave_wallet/browser/hd_keyring.h" #include "brave/components/brave_wallet/browser/internal/hd_key_ed25519.h" @@ -29,13 +31,13 @@ class SolanaKeyring : public HDKeyring { std::optional AddNewHDAccount() override; void RemoveLastHDAccount() override; - std::string ImportAccount(const std::vector& keypair) override; + std::string ImportAccount(base::span keypair) override; bool RemoveImportedAccount(const std::string& address) override; std::string EncodePrivateKeyForExport(const std::string& address) override; std::vector SignMessage(const std::string& address, - const std::vector& message); + base::span message); static std::optional CreateProgramDerivedAddress( const std::vector>& seeds, diff --git a/components/brave_wallet/common/eth_sign_typed_data_helper.cc b/components/brave_wallet/common/eth_sign_typed_data_helper.cc index 2410f4df8d96..739507918d46 100644 --- a/components/brave_wallet/common/eth_sign_typed_data_helper.cc +++ b/components/brave_wallet/common/eth_sign_typed_data_helper.cc @@ -9,6 +9,7 @@ #include #include +#include "base/containers/span.h" #include "base/logging.h" #include "base/strings/strcat.h" #include "base/strings/string_number_conversions.h" @@ -383,8 +384,8 @@ EthSignTypedDataHelper::GetTypedDataPrimaryHash( // static std::optional> EthSignTypedDataHelper::GetTypedDataMessageToSign( - const std::vector& domain_hash, - const std::vector& primary_hash) { + base::span domain_hash, + base::span primary_hash) { if (domain_hash.empty() || primary_hash.empty()) { return std::nullopt; } diff --git a/components/brave_wallet/common/eth_sign_typed_data_helper.h b/components/brave_wallet/common/eth_sign_typed_data_helper.h index 19c8afe7ca7e..e64740551734 100644 --- a/components/brave_wallet/common/eth_sign_typed_data_helper.h +++ b/components/brave_wallet/common/eth_sign_typed_data_helper.h @@ -13,6 +13,7 @@ #include #include "base/containers/flat_map.h" +#include "base/containers/span.h" #include "base/gtest_prod_util.h" #include "base/values.h" @@ -41,8 +42,8 @@ class EthSignTypedDataHelper { const std::string& primary_type_name, const base::Value::Dict& data) const; static std::optional> GetTypedDataMessageToSign( - const std::vector& domain_hash, - const std::vector& primary_hash); + base::span domain_hash, + base::span primary_hash); std::optional, base::Value::Dict>> GetTypedDataPrimaryHash(const std::string& primary_type_name, const base::Value::Dict& message) const;