ARCH2.md (4863B)
1 # ARCH2: Refactoring for Abstraction and Type Safety 2 3 ## Overview 4 5 This document describes refactoring opportunities identified in the ppad-bolt3 6 library to reduce code duplication and improve type-level invariant 7 enforcement. 8 9 ## Goals 10 11 1. **Reduce duplication** in HTLC transaction builders and key derivation 12 2. **Enforce size invariants** on ByteString-based newtypes at construction 13 3. **Enforce range invariants** on numeric types (48-bit CommitmentNumber) 14 15 ## Non-Goals 16 17 - Changing the public API signatures (beyond adding smart constructors) 18 - Adding runtime overhead to hot paths 19 - Breaking existing tests 20 21 --- 22 23 ## 1. Size-Constrained ByteString Types 24 25 ### Current State 26 27 Several newtypes wrap `ByteString` without enforcing expected sizes: 28 29 | Type | Expected Size | 30 |---------------------|---------------| 31 | `Pubkey` | 33 bytes | 32 | `Point` | 33 bytes | 33 | `Seckey` | 32 bytes | 34 | `TxId` | 32 bytes | 35 | `PaymentHash` | 32 bytes | 36 | `PaymentPreimage` | 32 bytes | 37 | `PerCommitmentSecret` | 32 bytes | 38 39 ### Proposed Change 40 41 Add smart constructors that validate size, returning `Maybe`: 42 43 ```haskell 44 -- | Parse a 33-byte compressed public key. 45 pubkey :: BS.ByteString -> Maybe Pubkey 46 pubkey bs 47 | BS.length bs == 33 = Just (Pubkey bs) 48 | otherwise = Nothing 49 50 -- | Parse a 32-byte transaction ID. 51 txid :: BS.ByteString -> Maybe TxId 52 txid bs 53 | BS.length bs == 32 = Just (TxId bs) 54 | otherwise = Nothing 55 ``` 56 57 The raw constructors remain exported for internal use and for cases where 58 the caller has already validated the input (e.g., from cryptographic 59 operations that guarantee output size). 60 61 ### Impact 62 63 - Callers parsing external data get early validation 64 - No runtime overhead for internal construction paths 65 - Tests may need updates for smart constructor usage 66 67 --- 68 69 ## 2. CommitmentNumber Range Constraint 70 71 ### Current State 72 73 `CommitmentNumber` wraps `Word64` but semantically represents a 48-bit value 74 (max 2^48 - 1 = 281474976710655). 75 76 ### Proposed Change 77 78 Add a smart constructor that validates the range: 79 80 ```haskell 81 -- | Create a commitment number (must be < 2^48). 82 commitment_number :: Word64 -> Maybe CommitmentNumber 83 commitment_number n 84 | n <= 0xFFFFFFFFFFFF = Just (CommitmentNumber n) 85 | otherwise = Nothing 86 ``` 87 88 ### Impact 89 90 - Prevents invalid commitment numbers from propagating 91 - Minimal - most internal uses are already within range 92 93 --- 94 95 ## 3. HTLC Transaction Builder Abstraction 96 97 ### Current State 98 99 `build_htlc_timeout_tx` and `build_htlc_success_tx` in `Tx.hs` share ~80% 100 of their implementation: 101 102 - Same output script construction 103 - Same input/output structure 104 - Differ only in: locktime source, fee function 105 106 ### Proposed Change 107 108 Factor out a common helper: 109 110 ```haskell 111 -- Internal helper for HTLC transaction construction 112 build_htlc_tx_common 113 :: HTLCContext 114 -> Locktime -- ^ Locktime for the transaction 115 -> Satoshi -- ^ Fee to subtract 116 -> HTLCTx 117 118 build_htlc_timeout_tx :: HTLCContext -> HTLCTx 119 build_htlc_timeout_tx ctx = 120 let !fee = htlc_timeout_fee (hc_feerate ctx) (hc_features ctx) 121 !locktime = Locktime (unCltvExpiry $ htlc_cltv_expiry $ hc_htlc ctx) 122 in build_htlc_tx_common ctx locktime fee 123 124 build_htlc_success_tx :: HTLCContext -> HTLCTx 125 build_htlc_success_tx ctx = 126 let !fee = htlc_success_fee (hc_feerate ctx) (hc_features ctx) 127 in build_htlc_tx_common ctx (Locktime 0) fee 128 ``` 129 130 ### Impact 131 132 - Reduces code duplication 133 - Makes the difference between timeout/success explicit 134 - No API change 135 136 --- 137 138 ## 4. Key Derivation Abstraction 139 140 ### Current State 141 142 In `Keys.hs`, several derivation functions are structurally identical: 143 144 - `derive_local_htlcpubkey` / `derive_remote_htlcpubkey` 145 - `derive_local_delayedpubkey` / `derive_remote_delayedpubkey` 146 147 Each just unwraps a basepoint, calls `derive_pubkey`, and wraps in a 148 different newtype. 149 150 ### Proposed Change 151 152 Keep the explicit functions (they provide good type safety and 153 documentation) but have them delegate to a single internal helper pattern. 154 The current implementation already does this via `derive_pubkey`, so this 155 is more about recognizing the pattern is intentional rather than changing 156 it. 157 158 Alternatively, could add a type class but that adds complexity for minimal 159 gain. 160 161 ### Decision 162 163 **Keep as-is.** The "duplication" is intentional type safety - each function 164 has a distinct type signature that prevents mixing up local/remote keys. 165 The actual derivation logic (`derive_pubkey`) is already shared. 166 167 --- 168 169 ## Summary of Changes 170 171 | Area | Change | Priority | 172 |------|--------|----------| 173 | Types.hs | Smart constructors for sized ByteStrings | High | 174 | Types.hs | Smart constructor for CommitmentNumber | Medium | 175 | Tx.hs | Factor HTLC tx builder common code | Medium | 176 | Keys.hs | Keep as-is (already well-factored) | N/A |