// GOOD - immutable record with validation in compact constructor
public record Address(@JsonValue String value) {
    public Address {
        Objects.requireNonNull(value, "address");
        if (!HEX.matcher(value).matches()) {
            throw new IllegalArgumentException("Invalid address: " + value);
        }
        value = value.toLowerCase(Locale.ROOT);
    }
}

// BAD - mutable class for simple data
public class Address {
    private String value;
    public void setValue(String v) { this.value = v; }
}

// GOOD - expression form returns value
return switch (type) {
    case UINT -> decodeUint(data);
    case ADDRESS -> decodeAddress(data);
    case BOOL -> decodeBool(data);
};

// BAD - statement form with breaks
switch (type) {
    case UINT:
        return decodeUint(data);
    case ADDRESS:
        return decodeAddress(data);
    default:
        throw new IllegalArgumentException();
}

// GOOD - pattern matching in instanceof
if (value instanceof Address addr) {
    return addr.value();
}

// BAD - separate cast
if (value instanceof Address) {
    Address addr = (Address) value;
    return addr.value();
}

// GOOD - text block
private static final String ABI_JSON = """
        [
          {"type": "function", "name": "transfer"}
        ]
        """;

// BAD - concatenation
private static final String ABI_JSON =
    "[\n" +
    "  {\"type\": \"function\"}\n" +
    "]";

// GOOD - type obvious from RHS
var address = new Address("0x...");
var mapper = new ObjectMapper();
var logs = new ArrayList<LogEntry>();

// BAD - var obscures type
var result = process(input);  // What type is result?

// GOOD - explicit when unclear
TransactionReceipt result = process(input);

// GOOD
return topics.stream().map(Hash::new).toList();

// BAD
return topics.stream().map(Hash::new).collect(Collectors.toList());

// GOOD - explicit null check with message
Objects.requireNonNull(provider, "provider");

// GOOD - Optional for truly optional values
public Optional<Long> nonceOpt() { ... }

// BAD - nullable without documentation
public Address getAddress() { return address; }  // Can this be null?

// BAD - Optional.get() without check
return optional.get();  // Use orElseThrow() instead

// GOOD - fully typed
List<LogEntry> logs = new ArrayList<>();
Map<String, Object> params = new LinkedHashMap<>();

// BAD - raw types
List logs = new ArrayList();
Map params = new HashMap();

BraneException (base)
├── RpcException (JSON-RPC errors)
├── RevertException (contract reverts with decoded reason)
├── AbiEncodingException (encoding failures)
├── AbiDecodingException (decoding failures)
├── ChainMismatchException (wrong chain ID)
├── InvalidSenderException (signer mismatch)
└── TxnException (transaction failures)

// GOOD - wrap with context, preserve cause
try {
    return provider.send(method, params);
} catch (IOException e) {
    throw new RpcException(-32000, "Connection failed: " + endpoint, null, e);
}

// BAD - lose cause
catch (IOException e) {
    throw new RpcException(-32000, "Connection failed", null);
}

// BAD - swallow exception
catch (IOException e) {
    return null;  // Silent failure
}

// BAD - catches too much
try {
    process(input);
} catch (Exception e) {
    // What exception? NPE? IllegalArgument? OutOfMemory?
}

// GOOD - catch specific exceptions
try {
    process(input);
} catch (RpcException e) {
    handleRpcError(e);
} catch (AbiDecodingException e) {
    handleDecodingError(e);
}

// GOOD - AtomicReference for lazy initialization
private final AtomicReference<Long> cachedChainId = new AtomicReference<>();

public long getChainId() {
    Long cached = cachedChainId.get();
    if (cached != null) {
        return cached;
    }
    long actual = fetchChainId();
    cachedChainId.set(actual);
    return actual;
}

// BAD - not thread-safe
private Long cachedChainId;

public long getChainId() {
    if (cachedChainId == null) {
        cachedChainId = fetchChainId();  // Race condition
    }
    return cachedChainId;
}

// GOOD - virtual threads for I/O-bound work
try (var executor = Executors.newVirtualThreadPerTaskExecutor()) {
    futures.forEach(f -> executor.submit(f));
}

// BAD - platform thread pool for I/O
ExecutorService executor = Executors.newFixedThreadPool(10);

/**
 * Sends a signed transaction to the network and waits for confirmation.
 *
 * <p>This method blocks until the transaction is mined or timeout is reached.
 *
 * @param request the transaction parameters (to, value, data, gas settings)
 * @param timeoutMillis maximum time to wait for confirmation
 * @param pollIntervalMillis interval between receipt checks
 * @return the transaction receipt with status and logs
 * @throws RpcException if the RPC call fails
 * @throws RevertException if the transaction reverts
 * @throws IllegalArgumentException if request is null
 */
TransactionReceipt sendTransactionAndWait(
        TransactionRequest request,
        long timeoutMillis,
        long pollIntervalMillis);

/**
 * Binds a Java interface to a deployed contract.
 *
 * <p><strong>Example:</strong>
 * <pre>{@code
 * Erc20Contract usdc = BraneContract.bind(
 *         new Address("0xA0b8..."),
 *         abiJson,
 *         publicClient,
 *         walletClient,
 *         Erc20Contract.class);
 *
 * BigInteger balance = usdc.balanceOf(myAddress);
 * }</pre>
 */

// BAD
public void process(Address addr) {
    return addr.value();  // NPE if null
}

// GOOD
public void process(Address addr) {
    Objects.requireNonNull(addr, "addr");
    return addr.value();
}

// BAD - caller can modify internal state
public List<LogEntry> getLogs() {
    return logs;
}

// GOOD - defensive copy or unmodifiable
public List<LogEntry> getLogs() {
    return List.copyOf(logs);
}

// BAD - stream not closed
InputStream is = new FileInputStream(file);
byte[] data = is.readAllBytes();

// GOOD - try-with-resources
try (InputStream is = new FileInputStream(file)) {
    byte[] data = is.readAllBytes();
}

// BAD
List<String> result = new ArrayList<>();
for (Hash h : hashes) {
    result.add(h.value());
}
return result;

// GOOD
return hashes.stream().map(Hash::value).toList();

// BAD
if (data.length() != 42) { ... }

// GOOD
private static final int ADDRESS_HEX_LENGTH = 42;  // "0x" + 40 hex chars
if (data.length() != ADDRESS_HEX_LENGTH) { ... }

// BAD - allocates on every call
public String getValue() {
    return "0x" + Hex.encode(bytes);
}

// GOOD - cache if immutable and frequently accessed
private final String cachedValue;

public String getValue() {
    return cachedValue;
}

// GOOD
long total = values.stream().mapToLong(Wei::toLong).sum();

// BAD - boxing overhead
Long total = values.stream().map(Wei::value).reduce(0L, Long::sum);

// GOOD
var sb = new StringBuilder();
for (var item : items) {
    sb.append(item.value());
}

// BAD
String result = "";
for (var item : items) {
    result += item.value();  // Creates new String each iteration
}

// GOOD - use BigInteger for Ethereum values
BigInteger value = new BigInteger(hexValue, 16);

// BAD - overflow risk with long
long value = Long.parseLong(hexValue, 16);  // Overflow if > Long.MAX_VALUE

@Test
void shouldRejectNullAddress() {
    // This test demonstrates the bug: NPE is thrown instead of IllegalArgumentException
    var client = new DefaultClient(provider);

    // EXPECTED: IllegalArgumentException with message "address required"
    // ACTUAL: NullPointerException at line 47
    assertThrows(IllegalArgumentException.class, () -> client.call(null));
}

EXECUTION TRACE:
1. User calls: client.call(null)
2. → DefaultClient.call(Address addr) at line 42
3. → [NO null check] proceeds to line 47
4. → addr.value() called at line 47
5. → NullPointerException thrown (unintended)

EXPECTED: IllegalArgumentException at step 3
ACTUAL: NPE at step 5

POTENTIAL BUG: Race condition in cached chain ID

CODE PATH:
1. Thread A calls getChainId(), sees cachedChainId == null
2. Thread A enters if-block, starts fetchChainId() (slow RPC call)
3. Thread B calls getChainId(), sees cachedChainId == null (not set yet)
4. Thread B also enters if-block, starts fetchChainId()
5. Both threads make redundant RPC calls

TRIGGER SCENARIO:
- Multiple virtual threads calling getChainId() on cold start
- High latency RPC endpoint (>100ms)

WHY TESTS MISS IT:
- Unit tests are single-threaded
- Integration tests use fast local Anvil, hiding the race window

DESIGN CONCERN: Method does too many things (violates SRP)

WHAT: sendTransactionAndWait() handles signing, sending, polling, and timeout

WHY IT MATTERS:
- Hard to test individual behaviors
- Timeout logic duplicated if user wants different polling strategy
- 150 lines in single method reduces readability

REFERENCE: See viem's separation: signTransaction() + sendRawTransaction() + waitForTransactionReceipt()

SUGGESTION: Use switch expression instead of if-else chain at line 87
Currently 15 lines, could be 8 with switch expression for better readability.

FINDING: Null pointer risk in processTransaction(tx)

EVIDENCE: [execution trace as above]

COUNTER-ARGUMENT (Why this might NOT be a bug):
- The public API `submitTransaction()` validates tx != null before calling processTransaction()
- processTransaction() is private and only called from submitTransaction()
- Therefore, null can never reach this code path in practice

VERDICT: After tracing all call sites, confirmed processTransaction() is ONLY called from
submitTransaction() which has null check. This is NOT a bug - the internal method can
safely assume non-null. Downgrading to T4 Suggestion: add @Nullable annotation or
Objects.requireNonNull for defensive coding.

# Find tests for the class under review
./gradlew test --tests "*ClassName*" --dry-run

# Search for test methods covering the specific functionality
grep -r "methodName\|ClassName" */src/test/

EXISTING TEST ANALYSIS:
- Found: DefaultClientTest.java lines 45-67 test call() method
- Coverage gap: Tests only pass Address objects, never test null input
- Conclusion: Bug is real, tests have coverage gap

EXISTING TEST ANALYSIS:
- Found: DefaultClientTest.shouldRejectNullAddress() at line 89
- Test passes and expects IllegalArgumentException
- Re-checking my analysis...

CORRECTION: I misread the code. Null check exists at line 41, I was looking at wrong method.
No bug here.

### [T1/T2/T3/T4] [Short Title]

**Location**: `module/path/to/File.java:LINE`

**Classification**: [Confirmed Bug | Potential Bug | Design Concern | Suggestion]

**Description**:
[1-2 sentences describing the issue]

**Evidence**:
[For T1: Failing test or execution trace]
[For T2: Code path + trigger scenario + test gap analysis]
[For T3: What + Why + Reference]
[For T4: Brief explanation]

**Existing Test Analysis**: (Required for T1/T2)
[What tests exist? Why don't they catch this?]

**Counter-Argument**: (Required for T1/T2)
[Why might this NOT be a bug? What would make my analysis wrong?]

**Verdict**:
[Final assessment after considering counter-argument]

**Recommended Fix**: (If applicable)
[Code snippet or description of fix]

BAD: "This might have null pointer issues"
GOOD: "Line 47 dereferences `addr.value()` without null check. Trace shows
       null can reach here via path X→Y→Z"

BAD: "The cache isn't thread-safe"
GOOD: "The cache uses plain field without synchronization. Trace shows Thread A
       can see partial write from Thread B when [specific scenario]"

BAD: Claim bug exists without checking if tests cover it
GOOD: "Tests exist but don't cover this case because [reason]"

BAD: "This input validation is missing" (without checking all entry points)
GOOD: "Checked all 3 call sites: submitTx(), batchSubmit(), internal retry().
       Only retry() lacks validation, but it's only called after validation
       in submitTx(). Not a bug."