T2972
Improve miner parsing.
- Adds more miners
- Updates the schema for miners in config.js for better unit testing
- Implements unit testing
A few steps, but imo made sense to do this in one shot.
Details
Details
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- herald-miner-parsing
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 23667 Build 46948: Build Diff ecash-herald-tests Build 46947: arc lint + arc unit
Event Timeline
| apps/ecash-herald/config.js | ||
|---|---|---|
| 50 | The test vectors have nothing to do in config.js | |
| apps/ecash-herald/config.js | ||
|---|---|---|
| 50 | This isn't the only thing currently living in config.js that isn't really a config and should require a diff to change. Could move this and also the opReturn constants into their own files. imo that's beyond the scope of this diff. I've added a task to improve this organization T3162 I'd like to keep the test vectors in the same place as the miners, so it's clear that adding another miner requires adding associated test blocks in this format. | |
| apps/ecash-herald/src/utils.js | ||
| 208 ↗ | (On Diff #40239) | maybe this should be in ecashaddrjs but this is the first time i've needed this method. will add it if it comes up again. |
| apps/ecash-herald/src/parse.js | ||
|---|---|---|
| 48 | You should pass this as a parameter. That makes the test easier as you can craft any tx/pattern you want and make sure you have coverage for all the detection methods. | |
| 73 | This whole IFP match is totally useless. You're losing more time with this branch than it takes to just fail matching the IFP script. | |
| 173 | If that's not going to be reused anywhere it's not worth making it a function. It makes testing less efficient as you don't supply a list of the know miners | |
| apps/ecash-herald/src/utils.js | ||
| 208 | This is a copy paste of the same function in alias server. | |
| apps/ecash-herald/config.js | ||
|---|---|---|
| 50 |
Agreed, and that should be done first so that this diff can be rebased on top.
That rational makes zero sense. The whole point of unit tests is to make sure the function does what it claims. The unit tests are not supposed to run on every single possible case ! You're testing some happy paths and the edges cases, then the function does always the same job so unless there is a new feature, you don't need more test. You're not testing block parsing on the whole blockchain, and update the test vectors for every new block right ? | |
| apps/ecash-herald/src/utils.js | ||
| 208 ↗ | (On Diff #40239) | That's wrong and that's my point. It's the second time already, and this is causing code duplication + test duplication. This is not OK. |
| apps/ecash-herald/config.js | ||
|---|---|---|
| 50 | Adding another miner could require changes to the getMinerFromCoinbaseTx function. Need a unit test to prove getMinerFromCoinbaseTx works with an added miner. We may or may not have a block in blocks.js for this new miner; for example I don't think it's worth adding a new block to blocks.js just to cover the lone zpool block we have seen so far. But we do need a unit test to prove that getMinerFromCoinbaseTx correctly recognizes zpool miner. Will shelf this diff while I work on some of the pre-reqs. What kind of organization would you like to see here? | |
| apps/ecash-herald/src/utils.js | ||
| 208 | ||