|
| 1 | +# Addressing PR #925 Weaknesses - Action Plan |
| 2 | + |
| 3 | +## Overview |
| 4 | +This document outlines how to address the three identified weaknesses in PR #925: |
| 5 | +1. Batch optimizer incompleteness |
| 6 | +2. Missing unit tests |
| 7 | +3. GPG signature configuration |
| 8 | + |
| 9 | +--- |
| 10 | + |
| 11 | +## ✅ COMPLETED: Unit Tests |
| 12 | + |
| 13 | +### What We Did |
| 14 | +Created comprehensive unit test suites covering all new features: |
| 15 | + |
| 16 | +**Files Created:** |
| 17 | +- `tests/unit/rate-limit.test.ts` - Rate limiting logic tests |
| 18 | +- `tests/unit/batch-optimizer.test.ts` - Batch optimizer utility tests |
| 19 | +- `tests/unit/wallet-nonce-atomic.test.ts` - Atomic nonce operation tests |
| 20 | + |
| 21 | +**Test Coverage:** |
| 22 | +- ✅ Per-IP rate limit calculation with edge cases |
| 23 | +- ✅ BigInt formatting helpers (formatWei, formatPercent) |
| 24 | +- ✅ Gas estimation savings calculations |
| 25 | +- ✅ Batch ID generation |
| 26 | +- ✅ Atomic nonce update logic |
| 27 | +- ✅ Race condition prevention validation |
| 28 | + |
| 29 | +**To Run Tests:** |
| 30 | +```bash |
| 31 | +npm run test:unit |
| 32 | +# or for specific tests: |
| 33 | +npm run test:unit -- tests/unit/rate-limit.test.ts |
| 34 | +``` |
| 35 | + |
| 36 | +--- |
| 37 | + |
| 38 | +## 🔧 TODO: GPG Signature Configuration |
| 39 | + |
| 40 | +### Option 1: Enable GPG Signing (Recommended for Production) |
| 41 | + |
| 42 | +#### Step 1: Generate GPG Key (if you don't have one) |
| 43 | +```bash |
| 44 | +# Generate a new GPG key |
| 45 | +gpg --full-generate-key |
| 46 | +# Choose: RSA and RSA, 4096 bits, no expiration |
| 47 | + |
| 48 | +# List your keys to get the key ID |
| 49 | +gpg --list-secret-keys --keyid-format=long |
| 50 | +# Output will show: sec rsa4096/<KEY_ID> ... |
| 51 | +``` |
| 52 | + |
| 53 | +#### Step 2: Configure Git to Use GPG |
| 54 | +```bash |
| 55 | +# Set your GPG key for signing |
| 56 | +git config --global user.signingkey <KEY_ID> |
| 57 | + |
| 58 | +# Enable GPG signing for all commits |
| 59 | +git config --global commit.gpgsign true |
| 60 | + |
| 61 | +# Set GPG program (if needed on Windows) |
| 62 | +git config --global gpg.program "C:/Program Files (x86)/GnuPG/bin/gpg.exe" |
| 63 | +``` |
| 64 | + |
| 65 | +#### Step 3: Add GPG Key to GitHub |
| 66 | +```bash |
| 67 | +# Export your public key |
| 68 | +gpg --armor --export <KEY_ID> |
| 69 | + |
| 70 | +# Copy the output and add to GitHub: |
| 71 | +# Settings → SSH and GPG keys → New GPG key |
| 72 | +``` |
| 73 | + |
| 74 | +#### Step 4: Re-sign Existing Commits |
| 75 | +```bash |
| 76 | +# Re-sign the last N commits |
| 77 | +git rebase --exec 'git commit --amend --no-edit -n -S' -i HEAD~11 |
| 78 | + |
| 79 | +# Force push to update remote |
| 80 | +git push --force-with-lease origin feat/critical-fixes-and-batch-optimizer |
| 81 | +``` |
| 82 | + |
| 83 | +### Option 2: Keep Unsigned (Acceptable for Open Source) |
| 84 | + |
| 85 | +Many open source projects don't require GPG signatures. If thirdweb-dev/engine doesn't enforce it, this is fine. |
| 86 | + |
| 87 | +**To Check Repository Policy:** |
| 88 | +```bash |
| 89 | +# Check if branch protection requires signed commits |
| 90 | +# Go to: https://github.com/thirdweb-dev/engine/settings/branches |
| 91 | +``` |
| 92 | + |
| 93 | +--- |
| 94 | + |
| 95 | +## 🚀 TODO: Complete Batch Optimizer Implementation |
| 96 | + |
| 97 | +### Current State: Preview/Demonstration |
| 98 | +- ✅ Gas price analysis - WORKING |
| 99 | +- ✅ Cost estimation - WORKING |
| 100 | +- ✅ Batch metadata caching - WORKING |
| 101 | +- ⚠️ Gas estimation - Uses averages (71k/tx) |
| 102 | +- ⚠️ Execute endpoint - Doesn't queue transactions |
| 103 | +- ⚠️ Status tracking - Returns placeholders |
| 104 | + |
| 105 | +### Option A: Keep as Preview (Current Approach) |
| 106 | +**Pros:** |
| 107 | +- Provides valuable cost estimation |
| 108 | +- Demonstrates API design |
| 109 | +- Transparent about limitations |
| 110 | +- Can merge PR now |
| 111 | + |
| 112 | +**Cons:** |
| 113 | +- Not production-ready for execution |
| 114 | +- Requires future PR to complete |
| 115 | + |
| 116 | +**Action:** Already documented in commit dc5714f ✅ |
| 117 | + |
| 118 | +--- |
| 119 | + |
| 120 | +### Option B: Complete Full Implementation |
| 121 | + |
| 122 | +If you want to make batch optimizer fully functional, here's the implementation plan: |
| 123 | + |
| 124 | +#### Phase 1: Real Gas Estimation |
| 125 | +**File:** `src/server/routes/transaction/batch-optimizer.ts` |
| 126 | + |
| 127 | +Replace lines 153-161 with: |
| 128 | +```typescript |
| 129 | +const estimateGasForBatch = async ( |
| 130 | + chainId: number, |
| 131 | + fromAddress: string, |
| 132 | + transactions: any[], |
| 133 | +): Promise<bigint> => { |
| 134 | + const chain = await getChain(chainId); |
| 135 | + const rpcRequest = getRpcClient({ client: thirdwebClient, chain }); |
| 136 | + |
| 137 | + let totalGas = 0n; |
| 138 | + for (const tx of transactions) { |
| 139 | + const gasEstimate = await estimateGas({ |
| 140 | + transaction: prepareContractCall({ |
| 141 | + client: thirdwebClient, |
| 142 | + chain, |
| 143 | + to: tx.to as Address, |
| 144 | + data: tx.data, |
| 145 | + value: tx.value ? BigInt(tx.value) : 0n, |
| 146 | + }), |
| 147 | + from: fromAddress as Address, |
| 148 | + }); |
| 149 | + totalGas += gasEstimate; |
| 150 | + } |
| 151 | + return totalGas; |
| 152 | +}; |
| 153 | +``` |
| 154 | + |
| 155 | +#### Phase 2: Queue Integration |
| 156 | +**File:** `src/server/routes/transaction/batch-optimizer.ts` |
| 157 | + |
| 158 | +Replace lines 377-399 with: |
| 159 | +```typescript |
| 160 | +// Import at top |
| 161 | +import { queueTx } from "../../../shared/db/transactions/queue-tx"; |
| 162 | + |
| 163 | +// In executeBatchTransactions handler: |
| 164 | +const queueIds: string[] = []; |
| 165 | + |
| 166 | +for (let i = 0; i < transactions.length; i++) { |
| 167 | + const tx = transactions[i]; |
| 168 | + |
| 169 | + const queueId = await queueTx({ |
| 170 | + queueId: `${batchId}_tx_${i}`, |
| 171 | + tx: { |
| 172 | + to: tx.to as Address, |
| 173 | + from: fromAddress as Address, |
| 174 | + data: tx.data, |
| 175 | + value: tx.value ? BigInt(tx.value) : 0n, |
| 176 | + }, |
| 177 | + chainId: chainId.toString(), |
| 178 | + extension: "none", |
| 179 | + functionName: "batchTransaction", |
| 180 | + }); |
| 181 | + |
| 182 | + queueIds.push(queueId); |
| 183 | +} |
| 184 | + |
| 185 | +// Update batch status |
| 186 | +await cacheBatchData(batchId, { |
| 187 | + ...batchData, |
| 188 | + status: "queued", |
| 189 | + queueIds, |
| 190 | + queuedAt: Date.now(), |
| 191 | +}); |
| 192 | + |
| 193 | +reply.status(StatusCodes.OK).send({ |
| 194 | + batchId, |
| 195 | + status: "queued", |
| 196 | + message: `Batch of ${transactions.length} transactions queued for execution with ${optimization} optimization`, |
| 197 | + queueIds, |
| 198 | +}); |
| 199 | +``` |
| 200 | + |
| 201 | +#### Phase 3: Status Tracking |
| 202 | +**File:** `src/server/routes/transaction/batch-optimizer.ts` |
| 203 | + |
| 204 | +Replace lines 433-450 with: |
| 205 | +```typescript |
| 206 | +const txStatuses = await Promise.all( |
| 207 | + queueIds.map(async (queueId: string) => { |
| 208 | + const tx = await prisma.transactions.findUnique({ |
| 209 | + where: { queueId }, |
| 210 | + select: { |
| 211 | + status: true, |
| 212 | + transactionHash: true, |
| 213 | + minedAt: true, |
| 214 | + errorMessage: true, |
| 215 | + }, |
| 216 | + }); |
| 217 | + |
| 218 | + if (!tx) { |
| 219 | + return { queueId, status: "pending", transactionHash: undefined }; |
| 220 | + } |
| 221 | + |
| 222 | + return { |
| 223 | + queueId, |
| 224 | + status: tx.minedAt ? "mined" : |
| 225 | + tx.errorMessage ? "failed" : |
| 226 | + "processing", |
| 227 | + transactionHash: tx.transactionHash || undefined, |
| 228 | + }; |
| 229 | + }) |
| 230 | +); |
| 231 | + |
| 232 | +const completedCount = txStatuses.filter(t => t.status === "mined").length; |
| 233 | +const failedCount = txStatuses.filter(t => t.status === "failed").length; |
| 234 | + |
| 235 | +const response = { |
| 236 | + batchId, |
| 237 | + status: failedCount > 0 ? "failed" : |
| 238 | + completedCount === transactions.length ? "completed" : |
| 239 | + "processing", |
| 240 | + transactionCount: transactions.length, |
| 241 | + completedCount, |
| 242 | + failedCount, |
| 243 | + transactions: txStatuses, |
| 244 | +} satisfies Static<typeof batchStatusSchema>; |
| 245 | +``` |
| 246 | + |
| 247 | +#### Phase 4: Testing |
| 248 | +Create integration tests: |
| 249 | +```bash |
| 250 | +# Create test file |
| 251 | +touch tests/e2e/batch-optimizer.e2e.test.ts |
| 252 | +``` |
| 253 | + |
| 254 | +#### Phase 5: Update Documentation |
| 255 | +Update `docs/BATCH_OPTIMIZER.md`: |
| 256 | +- Remove "⚠️ Preview Status" section |
| 257 | +- Update status from "Preview" to "Production Ready" |
| 258 | +- Add production deployment notes |
| 259 | + |
| 260 | +--- |
| 261 | + |
| 262 | +## 📋 Recommended Next Steps |
| 263 | + |
| 264 | +### For This PR (Immediate): |
| 265 | +1. ✅ **Unit tests** - DONE (files created above) |
| 266 | +2. ⏭️ **GPG signing** - Decide if required (check with team) |
| 267 | +3. ⏭️ **Batch optimizer** - Keep as preview with documentation |
| 268 | + |
| 269 | +### For Follow-up PR (Future): |
| 270 | +1. Complete batch optimizer implementation (Phases 1-5 above) |
| 271 | +2. Add integration tests |
| 272 | +3. Performance benchmarking |
| 273 | +4. Load testing for batch operations |
| 274 | + |
| 275 | +--- |
| 276 | + |
| 277 | +## 🎯 Decision Matrix |
| 278 | + |
| 279 | +| Weakness | Current PR | Follow-up PR | Skip | |
| 280 | +|----------|-----------|--------------|------| |
| 281 | +| **Unit Tests** | ✅ DONE | | | |
| 282 | +| **GPG Signatures** | | ✅ If required | ✅ If not enforced | |
| 283 | +| **Batch Optimizer** | ✅ Keep preview | ✅ Complete impl | | |
| 284 | + |
| 285 | +--- |
| 286 | + |
| 287 | +## 💡 Recommendation |
| 288 | + |
| 289 | +**For PR #925:** |
| 290 | +- ✅ Merge with unit tests added |
| 291 | +- ✅ Keep batch optimizer as documented preview |
| 292 | +- ⏭️ Address GPG only if repository policy requires it |
| 293 | + |
| 294 | +**Rationale:** |
| 295 | +- Fixes 5 critical production bugs (high priority) |
| 296 | +- Adds valuable monitoring capabilities |
| 297 | +- Batch optimizer provides real value in preview mode |
| 298 | +- Clean separation of concerns (bug fixes now, full features later) |
| 299 | + |
| 300 | +**Next PR Focus:** |
| 301 | +- Complete batch optimizer implementation |
| 302 | +- Add integration tests |
| 303 | +- Performance optimization |
| 304 | + |
| 305 | +--- |
| 306 | + |
| 307 | +## 📝 Checklist Before Merge |
| 308 | + |
| 309 | +- [x] All unit tests created and passing |
| 310 | +- [x] Critical bugs fixed and tested |
| 311 | +- [x] Batch optimizer limitations documented |
| 312 | +- [x] API documentation complete |
| 313 | +- [ ] GPG signatures (if required by repo policy) |
| 314 | +- [x] Git history clean and professional |
| 315 | +- [x] No breaking changes |
| 316 | +- [x] Backward compatible |
| 317 | + |
| 318 | +--- |
| 319 | + |
| 320 | +## 🔗 Related Files |
| 321 | + |
| 322 | +- **Unit Tests:** `tests/unit/rate-limit.test.ts`, `tests/unit/batch-optimizer.test.ts`, `tests/unit/wallet-nonce-atomic.test.ts` |
| 323 | +- **Documentation:** `docs/BATCH_OPTIMIZER.md` |
| 324 | +- **Implementation:** `src/server/routes/transaction/batch-optimizer.ts` |
| 325 | +- **This Plan:** `ADDRESSING_WEAKNESSES.md` |
0 commit comments