AI Code Review - Claude Code 2025

libcardano C++20 SDK Code Review

Code Quality Analysis Date: December 6, 2025

---

Executive Summary

This document contains a comprehensive code review of the libcardano C++20 blockchain SDK, identifying 14 concrete improvements across code quality, performance, maintainability, and modern C++ best practices.

Priority Breakdown

• High Priority (Immediate): 4 issues
• Medium Priority (Short-term): 5 issues
• Low Priority (Long-term): 5 issues

---

High Priority Issues

1. O(N×M) Dictionary Lookup Performance

File: src/mnemonic.cpp (Lines 93-104) Category: Performance Priority: HIGH

Issue

Linear search through 2048-word BIP39 dictionary for each word in the mnemonic results in O(n*m) complexity. For a 24-word mnemonic, this performs up to 48,576 comparisons.

Current Code

for (const auto &w : this->word_list_)
{
    for (uint16_t i = 0; i < d.size(); i++)  // O(2048) each iteration
    {
        if (d[i] == w)
        {
            this->word_indexes_.push_back(i);
            break;
        }
    }
}

Recommended Fix

auto createDictionaryLookup(const std::span<const std::string_view> dict)
    -> std::unordered_map<std::string_view, uint16_t>
{
    auto lookup = std::unordered_map<std::string_view, uint16_t>();
    lookup.reserve(dict.size());
    for (uint16_t i = 0; i < dict.size(); i++)
        lookup[dict[i]] = i;
    return lookup;
}

// Usage: O(n) hash lookups instead of O(n*m) linear search
auto lookup = createDictionaryLookup(d);
for (const auto &w : this->word_list_)
{
    auto it = lookup.find(w);
    if (it != lookup.end())
        this->word_indexes_.push_back(it->second);
    else
        throw std::invalid_argument("Word not found in dictionary");
}

Benefit

• Reduces complexity from O(n*m) to O(n)
• ~2000× performance improvement for mnemonic validation
• Reduces 48,576 comparisons to 24 hash lookups for 24-word phrase

---

2. Mutable State in Conceptually Const Method

File: src/shelley/transaction.cpp (Lines 405-430) Category: API Design Priority: HIGH

Issue

The calculateFee() method modifies internal transaction state by adding and removing witnesses, even though conceptually it should be a read-only operation. This violates the principle of least surprise and creates unexpected side effects.

Current Code

auto TransactionBuilder::calculateFee(uint32_t numWitnesses) -> uint64_t
{
    // Modifies transaction state!
    this->clearWitnessSet();
    for (uint32_t i = 0; i < numWitnesses; ++i)
    {
        this->tx_.transaction_witness_set.vkeywitnesses.push_back(...);
    }

    auto calculated_fee = /* ... */;

    // Modifies state again!
    this->clearWitnessSet();

    return calculated_fee;
}

Recommended Fix

auto TransactionBuilder::calculateFee(uint32_t numWitnesses) const -> uint64_t
{
    const auto a = static_cast<uint64_t>(this->min_fee_a_);
    const auto b = static_cast<uint64_t>(this->min_fee_b_);

    // Create a temporary copy - don't modify original
    auto temp_tx = this->tx_;

    // Add dummy vkey witnesses to the copy
    for (uint32_t i = 0; i < numWitnesses; ++i)
    {
        const auto dummy_key = util::makeRandomByteArray<32>();
        const auto dummy_witness = util::makeRandomByteArray<64>();
        temp_tx.transaction_witness_set.vkeywitnesses.push_back(
            {dummy_key, dummy_witness}
        );
    }

    // Calculate fee using temporary transaction
    auto calculated_fee = a * temp_tx.serialize().size() + b;

    while (calculated_fee != temp_tx.transaction_body.fee)
    {
        temp_tx.transaction_body.fee = calculated_fee;
        calculated_fee = a * temp_tx.serialize().size() + b;
    }

    return calculated_fee;
}

Benefit

• Makes the method truly const - no side effects
• Improves code clarity and predictability
• Allows method to be called on const instances
• Better thread safety
• Follows principle of least surprise

---

3. Throwing Generic std::exception with No Message

File: src/bip32_ed25519.cpp (Line 202) Category: Error Handling Priority: HIGH

Issue

Throwing generic std::exception() provides no error message and is not best practice. Users cannot determine what went wrong without examining the source code.

Current Code

auto PublicKey::deriveChild(const uint32_t index, const DerivationMode mode)
    const -> PublicKey
{
    if (index_is_hardened(index)) throw std::exception();
    // ...
}

Recommended Fix

auto PublicKey::deriveChild(const uint32_t index, const DerivationMode mode)
    const -> PublicKey
{
    if (index_is_hardened(index))
    {
        throw std::invalid_argument(
            "Cannot derive hardened child from public key. "
            "Hardened derivation requires private key. "
            "Index: " + std::to_string(index)
        );
    }
    // ...
}

Benefit

• Provides clear, actionable error messages
• Helps users understand what went wrong and how to fix it
• Better debugging experience
• Uses appropriate exception type (std::invalid_argument)

---

4. No File Operation Error Checking

File: src/util.cpp (Lines 28-42) Category: Error Handling Priority: HIGH

Issue

No error checking if file opened successfully, if writes succeed, or if close() succeeded. This can lead to silent failures and data loss.

Current Code

auto cardano::util::writeEnvelopeTextFile(
    const std::string_view file_path,
    const std::string_view type,
    const std::string_view description,
    const std::string_view cbor_hex
) -> void
{
    std::ofstream out(std::string(file_path).c_str());
    out << "{\n";  // No check if file opened!
    out << R"(    "type": ")" << type << "\",\n";
    out << R"(    "description": ")" << description << "\",\n";
    out << R"(    "cborHex": ")" << cbor_hex << "\"\n";
    out << "}";
    out.close();  // Explicit close, but no error checking
}

Recommended Fix

auto cardano::util::writeEnvelopeTextFile(
    const std::string_view file_path,
    const std::string_view type,
    const std::string_view description,
    const std::string_view cbor_hex
) -> void
{
    auto out = std::ofstream(std::string(file_path));

    if (!out)
    {
        throw std::runtime_error(
            "Failed to open file for writing: " + std::string(file_path)
        );
    }

    // Enable exceptions on failure
    out.exceptions(std::ofstream::failbit | std::ofstream::badbit);

    try
    {
        out << "{\n";
        out << R"(    "type": ")" << type << "\",\n";
        out << R"(    "description": ")" << description << "\",\n";
        out << R"(    "cborHex": ")" << cbor_hex << "\"\n";
        out << "}";
        // Destructor will close and flush
    }
    catch (const std::ofstream::failure& e)
    {
        throw std::runtime_error(
            "Failed to write to file " + std::string(file_path) +
            ": " + e.what()
        );
    }
}

Benefit

• Proper error handling for file operations
• Clear error messages with context
• RAII handles resource cleanup automatically
• Prevents silent failures and data loss
• Detects disk full, permission denied, etc.

---

Medium Priority Issues

5. Duplicate Code in Address Classes

File: src/address.cpp (Lines 93-115, 167-181, 232-246) Category: Code Duplication Priority: MEDIUM

Issue

The pattern of creating a Blake2b(224) hash and hashing a public key is repeated three times across different address classes (BaseAddress, EnterpriseAddress, RewardsAddress). This violates the DRY principle.

Current Code

// BaseAddress::fromKeys (lines 93-115)
const auto blake2b = Botan::HashFunction::create("Blake2b(224)");
const auto pmt_key_bytes = pmt_key.bytes();
blake2b->update(pmt_key_bytes.data(), pmt_key_bytes.size());
const auto pmt_key_hash = blake2b->final();

// Similar code repeated in EnterpriseAddress::fromKey
// Similar code repeated in RewardsAddress::fromKey

Recommended Fix

// Add to address.cpp anonymous namespace
namespace {
    auto hashPublicKey(const bip32_ed25519::PublicKey& key)
        -> std::array<uint8_t, KEY_HASH_LENGTH>
    {
        const auto blake2b = Botan::HashFunction::create("Blake2b(224)");
        const auto key_bytes = key.bytes();
        blake2b->update(key_bytes.data(), key_bytes.size());
        const auto hash = blake2b->final();
        return util::makeByteArray<KEY_HASH_LENGTH>(hash);
    }
}

// Usage in all three locations:
auto BaseAddress::fromKeys(/* ... */) -> BaseAddress
{
    return BaseAddress(nid, hashPublicKey(pmt_key), hashPublicKey(stake_key));
}

Benefit

• Reduces code duplication
• Improves maintainability
• Centralizes hashing logic
• Single point of change if algorithm needs updating

---

6. Misleading Error Message

File: src/mnemonic.cpp (Lines 105-106) Category: Error Handling Priority: MEDIUM

Issue

Error message says "Words and indexes must match lengths" when the actual problem is "Invalid word not found in BIP39 dictionary."

Current Code

if (this->word_list_.size() != this->word_indexes_.size())
    throw std::invalid_argument("Words and indexes must match lengths");

Recommended Fix

if (this->word_indexes_.size() != this->word_list_.size())
{
    throw std::invalid_argument(
        "Invalid mnemonic: " +
        std::to_string(this->word_list_.size() - this->word_indexes_.size()) +
        " word(s) not found in BIP39 dictionary"
    );
}

Benefit

• Provides clear, actionable error messages
• Helps users identify the actual problem
• Better user experience and faster debugging

---

7. Hardcoded Magic Numbers

File: src/address.cpp (Lines 43-55, 124, 149) Category: Maintainability Priority: MEDIUM

Issue

Magic numbers for bit shifts and masks appear throughout the address code without clear documentation. Some constants are defined but others are hardcoded inline.

Current Code

constexpr uint8_t NETWORK_TAG_MAINNET = 0b0001;
constexpr uint8_t NETWORK_TAG_TESTNET = 0b0000;
// ... but then later:
this->header_byte_ = (0b0110 << 4) | 0b0001;  // What do these mean?

Recommended Fix

namespace {
    // Address type masks and shifts
    constexpr uint8_t ADDRESS_TYPE_MASK = 0b11110000;
    constexpr uint8_t NETWORK_ID_MASK = 0b00001111;
    constexpr uint8_t ADDRESS_TYPE_SHIFT = 4;

    // Network tags
    constexpr uint8_t NETWORK_TAG_MAINNET = 0b0001;
    constexpr uint8_t NETWORK_TAG_TESTNET = 0b0000;

    // Address types
    constexpr uint8_t SHELLEY_ADDR_PAYMENTKEYHASH_STAKEKEYHASH = 0b0000;
    constexpr uint8_t SHELLEY_ADDR_PAYMENTKEYHASH = 0b0110;
    constexpr uint8_t STAKE_ADDR_STAKEKEYHASH = 0b1110;

    // Helper functions
    constexpr auto makeHeaderByte(uint8_t addr_type, uint8_t network_tag)
    {
        return (addr_type << ADDRESS_TYPE_SHIFT) | network_tag;
    }

    constexpr auto extractAddressType(uint8_t header_byte)
    {
        return header_byte >> ADDRESS_TYPE_SHIFT;
    }
}

// Usage:
this->header_byte_ = makeHeaderByte(SHELLEY_ADDR_PAYMENTKEYHASH, net_tag);

Benefit

• Eliminates magic numbers
• Improves code clarity
• Makes bit manipulation more understandable
• Helper functions encapsulate encoding/decoding logic

---

8. Potential Integer Overflow in Balance Calculation

File: src/shelley/transaction.cpp (Lines 256-261) Category: Correctness Priority: MEDIUM

Issue

Converting between uint64_t and int64_t without overflow checks. While unlikely with Cardano's supply cap, the pattern is fragile.

Current Code

auto& change_output = this->tx_.transaction_body.transaction_outputs.back();
change_output.amount = static_cast<uint64_t>(
    static_cast<int64_t>(change_output.amount) + change
);
if (static_cast<int64_t>(change_output.amount) < this->min_utxo_)

Recommended Fix

auto& change_output = this->tx_.transaction_body.transaction_outputs.back();

// Check for underflow before performing arithmetic
if (change < 0 && change_output.amount < static_cast<uint64_t>(-change))
{
    throw std::runtime_error(
        "Change output adjustment would cause underflow"
    );
}

change_output.amount = static_cast<uint64_t>(
    static_cast<int64_t>(change_output.amount) + change
);

if (change_output.amount < static_cast<uint64_t>(this->min_utxo_))
{
    this->tx_.transaction_body.transaction_outputs.pop_back();
}

Benefit

• Prevents potential arithmetic errors
• Adds explicit validation
• Makes the code more robust
• Better error messages for debugging

---

9. Inconsistent Error Messages for Invalid Arguments

File: src/address.cpp (Lines 122, 188, 253) Category: Error Handling Priority: MEDIUM

Issue

Three different address types all throw identical error messages despite expecting different data sizes. Users cannot determine which validation failed.

Current Code

// All three throw the same message:
throw std::runtime_error("Invalid Bech32 data.");

Recommended Fix

// BaseAddress::fromBech32
if (data.size() != KEY_HASH_LENGTH * 2 + 1)
{
    throw std::invalid_argument(
        "Invalid Bech32 data for BaseAddress: expected " +
        std::to_string(KEY_HASH_LENGTH * 2 + 1) + " bytes, got " +
        std::to_string(data.size()) + " bytes"
    );
}

// EnterpriseAddress::fromBech32
if (data.size() != KEY_HASH_LENGTH + 1)
{
    throw std::invalid_argument(
        "Invalid Bech32 data for EnterpriseAddress: expected " +
        std::to_string(KEY_HASH_LENGTH + 1) + " bytes, got " +
        std::to_string(data.size()) + " bytes"
    );
}

Benefit

• Provides specific, actionable error messages
• Helps users identify exactly what went wrong
• Shows expected vs actual sizes
• Improves debugging experience and API usability

---

Low Priority Issues

10. Missing noexcept on Move Operations

File: include/cardano/address.hpp (BaseAddress, EnterpriseAddress, etc.) Category: Modern C++ Priority: LOW

Issue

Classes containing only standard library containers lack explicit noexcept move operations. std::vector can't use move operations in reallocations unless they're noexcept.

Recommended Fix

class BaseAddress : public IShelleyAddress
{
public:
    BaseAddress(BaseAddress&&) noexcept = default;
    BaseAddress& operator=(BaseAddress&&) noexcept = default;

    BaseAddress(const BaseAddress&) = default;
    BaseAddress& operator=(const BaseAddress&) = default;

    ~BaseAddress() = default;
    // ...
};

Benefit

• Enables move semantics optimizations in standard containers
• std::vector can use move instead of copy during reallocation
• Explicitly documents move semantics behavior

---

11. Missing const Correctness in Utility Functions

File: src/util.cpp (Lines 44-105) Category: Code Quality Priority: LOW

Issue

Variables like 'neg' and 'i' are set once but not declared const.

Recommended Fix

const bool is_negative = (f < 0);
// Use throughout instead of 'neg' flag

Benefit

• Improves const correctness
• Makes code intent clearer
• Helps compiler optimize better

---

12. Missing nodiscard on Factory Methods

File: include/cardano/bip32_ed25519.hpp (Lines 170, 180, 186, 196) Category: Modern C++ Priority: LOW

Issue

Some factory methods have [[nodiscard]], others don't. Inconsistent.

Current Code

[[nodiscard]] static auto generate() -> PrivateKey;
[[nodiscard]] static auto fromSeed(/* ... */) -> PrivateKey;

// Missing [[nodiscard]]:
static auto fromMnemonic(const Mnemonic& mn) -> PrivateKey;
static auto fromMnemonicByron(const Mnemonic& mn) -> PrivateKey;

Recommended Fix

[[nodiscard]] static auto fromMnemonic(const Mnemonic& mn) -> PrivateKey;
[[nodiscard]] static auto fromMnemonicByron(const Mnemonic& mn) -> PrivateKey;

Benefit

• Prevents accidentally discarding expensive cryptographic operations
• Consistent API design
• Compiler warnings if return values are ignored

---

13. Misleading std::move on std::array

File: src/address.cpp (Lines 84, 159, 224) Category: Code Quality Priority: LOW

Issue

Using std::move() on std::array doesn't provide any benefit since arrays are fixed-size and moving them is equivalent to copying.

Current Code

this->pmt_key_hash_ = std::move(pmt_key_hash);  // No benefit

Recommended Fix

BaseAddress::BaseAddress(
    NetworkID nid,
    const std::array<uint8_t, KEY_HASH_LENGTH>& pmt_key_hash,
    const std::array<uint8_t, KEY_HASH_LENGTH>& stake_key_hash
)
{
    this->pmt_key_hash_ = pmt_key_hash;
    this->stk_key_hash_ = stake_key_hash;
    // ...
}

Benefit

• Removes misleading use of std::move
• Clearer intent
• Better understanding of value semantics

---

14. Recursion Guard Without Clear Error Message

File: src/bip32_ed25519.cpp (Lines 107-133) Category: Correctness Priority: LOW

Issue

Guard checks count > 1000 but error says "looping forever" which is misleading.

Current Code

if (count > 1000)
    throw std::runtime_error("Cannot generate root key (looping forever).");

Recommended Fix

constexpr size_t MAX_ITERATIONS = 1000;

if (count > MAX_ITERATIONS)
{
    throw std::runtime_error(
        "Cannot generate root key: maximum iterations (" +
        std::to_string(MAX_ITERATIONS) + ") exceeded. "
        "This indicates either a corrupted seed or an implementation error."
    );
}

Benefit

• Better error message explains the problem
• Makes the guard more explicit
• Documents expected behavior

---

Implementation Priority

Immediate (High Priority) - Implement First

1. Dictionary lookup performance (2000× improvement)
2. Fix calculateFee const correctness
3. Replace generic exceptions with specific ones
4. Add file operation error handling

Short-Term (Medium Priority) - Next Sprint

5. Extract duplicate Blake2b hashing
6. Improve error messages in mnemonic
7. Replace magic numbers with named constants
8. Add overflow protection in balance calculation
9. Make error messages consistent and specific

Long-Term (Low Priority) - Nice to Have

10. Add noexcept to move operations
11. Improve const correctness in util functions
12. Add nodiscard consistently
13. Remove misleading std::move usage
14. Clarify recursion guards

---

Conclusion

This code review identified 14 concrete improvements across various aspects of code quality:

• Performance: 1 critical issue (2000× improvement possible)
• Error Handling: 5 issues (better user experience)
• Code Duplication: 1 issue (maintainability)
• API Design: 1 issue (const correctness)
• Modern C++: 2 issues (noexcept, nodiscard)
• Maintainability: 2 issues (magic numbers, const)
• Correctness: 2 issues (overflow, recursion)

The codebase is generally well-structured and follows many C++20 best practices. The identified improvements will enhance performance, error handling, and maintainability without requiring major architectural changes.

Most improvements are straightforward to implement and have clear benefits. The high-priority issues should be addressed first as they have the most significant impact on performance and correctness.