Details
- Reviewers
Fabien - Group Reviewers
Restricted Project - Commits
- rABCd637f245a649: [herald] Improved miner parsing
npm test
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- herald-miner-parsing-next
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 23841 Build 47292: Build Diff ecash-herald-tests Build 47291: arc lint + arc unit
Event Timeline
| apps/ecash-herald/constants/miners.js | ||
|---|---|---|
| 64 | Test vectors do no belong in constants, but in (e.g.) test/miners.js or test/data/miners.js. This is not part of the application logic, it's a test fixture | |
| apps/ecash-herald/src/parse.js | ||
| 57 | You should break after that | |
| 94 | You are reinventing String.split() | |
| 123 | dito | |
| 131 | I don't really understand why this needs to be different from the viabtc parsing. The pattern is exactly the same afaict | |
| apps/ecash-herald/src/parse.js | ||
|---|---|---|
| 116 ↗ | (On Diff #40516) | I'm still unsure this is a good idea to make it a special case. Returning CKPool as a miner, with mined by Iceberg in the details is enough imo and will avoid this special case alltogether |
| apps/ecash-herald/src/parse.js | ||
|---|---|---|
| 116 ↗ | (On Diff #40516) | Yes, this is what is currently in prod. However we've had a task open for awhile to fix it: T3158 Most likely there is just one miner using CK pool software, and the rare blocks that don't include "iceberg" are anomalous. Since basically 100% of ck pool blocks are mined by iceberg, it makes sense to label them accordingly. |