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 23853 Build 47316: Build Diff ecash-herald-tests Build 47315: arc lint + arc unit
Event Timeline
| apps/ecash-herald/constants/miners.js | ||
|---|---|---|
| 64 ↗ | (On Diff #40503) | 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 ↗ | (On Diff #40503) | You should break after that |
| 94 ↗ | (On Diff #40503) | You are reinventing String.split() |
| 123 ↗ | (On Diff #40503) | dito |
| 131 ↗ | (On Diff #40503) | 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 | 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 | 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. | |