diff --git a/AGENTS.md b/AGENTS.md index 789a311001..668381828c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -31,6 +31,8 @@ See [CONTRIBUTING.md](./CONTRIBUTING.md#pre-commit-hooks) for `pre-commit` setup New file placement and header levelization: see [CONTRIBUTING.md](./CONTRIBUTING.md#before-making-a-pull-request). Braces, whitespace, member order, and other conventions: see [docs/CodingStyle.md](./docs/CodingStyle.md). `XRPL_ASSERT`/`UNREACHABLE` contracts: see [CONTRIBUTING.md](./CONTRIBUTING.md#contracts-and-instrumentation). Commit messages: see [CONTRIBUTING.md](./CONTRIBUTING.md#good-commit-messages). New public functions/methods need a Doxygen-style comment. +Comments should explain _why_, not _what_/_how_ — the code already shows that. Only describe what/how when the code itself would otherwise be confusing (a non-obvious workaround, a subtle invariant, a surprising constraint). + ## Architecture See [ARCHITECTURE.md](./ARCHITECTURE.md) for the directory-by-directory map of the codebase. diff --git a/src/libxrpl/ledger/helpers/AGENTS.md b/src/libxrpl/ledger/helpers/AGENTS.md index 615e24451b..03bb46f752 100644 --- a/src/libxrpl/ledger/helpers/AGENTS.md +++ b/src/libxrpl/ledger/helpers/AGENTS.md @@ -2,7 +2,7 @@ See the repo-level [AGENTS.md](../../../../AGENTS.md) for general guidance. -A helper that takes an `SLE`/`std::shared_ptr` should `XRPL_ASSERT` that it's non-null and of the expected ledger-entry type at entry, and keep a real runtime check/error-return alongside the assert (asserts compile out in release builds). Don't invent a new error-handling idiom for this (e.g. `std::unexpected`) — return the existing `tec`/`ter`/`tef` code used elsewhere in the codebase. +A helper that takes an `SLE`/`std::shared_ptr` should `XRPL_ASSERT` that it's non-null and of the expected ledger-entry type at entry, and keep a real runtime check/error-return alongside the assert (asserts compile out in release builds) — the established idiom in this directory is `std::expected<..., TER>`, returning `std::unexpected(tec*)` on failure. Don't invent a new error-handling idiom for this. Prefer a single amendment-enabled block and a single disabled block over scattering `rules.enabled(...)` checks through a function, even if the two blocks are similar. When a file or function checks more than one amendment, name local enablement booleans per-amendment (e.g. `fix340Enabled` for `fixCleanup3_4_0`), not a generic `fixEnabled`. diff --git a/src/libxrpl/tx/transactors/AGENTS.md b/src/libxrpl/tx/transactors/AGENTS.md index 9dbdc052c8..02a253c8ea 100644 --- a/src/libxrpl/tx/transactors/AGENTS.md +++ b/src/libxrpl/tx/transactors/AGENTS.md @@ -2,8 +2,6 @@ See [tx/AGENTS.md](../AGENTS.md) for amendment-gating conventions that apply to all transactors, and the repo-level [AGENTS.md](../../../../AGENTS.md) for general guidance. -Pseudo-accounts (Vault, LoanBroker, AMM, ...) are exempt from `requireAuth` and freeze/deep-freeze checks as a class, not on a per-asset-type basis. Code that touches a pseudo-account (deposits, withdrawals, clawback, deletion, credential checks) must preserve that exemption rather than re-deriving it for each asset type. - Prefer a single object-level invariant over duplicating the same delta/balance check in every transactor that touches an object — e.g. one invariant asserting a Vault's pseudo-account balance and `assetsAvailable` always move together, rather than repeating that check in `VaultDeposit`, `VaultWithdraw`, `VaultClawback`, `LoanSet`, etc. ## Gating amendment-dependent code @@ -12,4 +10,6 @@ Prefer a single amendment-enabled block and a single disabled block over scatter When a file or function checks more than one amendment, name local enablement booleans per-amendment (e.g. `fix340Enabled` for `fixCleanup3_4_0`), not a generic `fixEnabled` — it becomes ambiguous once a second amendment is checked in the same scope. +## `UNREACHABLE` and test coverage + Only use `UNREACHABLE` for genuinely impossible paths, not to avoid writing a test for one that's reachable but rare. When a branch marked `UNREACHABLE` is excluded from coverage, wrap it in `LCOV_EXCL_START`/`LCOV_EXCL_STOP`.