And attempt to guess the reason why it was rejected.
Details
- Reviewers
bytesofman - Group Reviewers
Restricted Project - Commits
- rABC43f19a37b28f: [herald] Show invalidated blocks
npm run test
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- herald_invalidated_block
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 30482 Build 60481: Build Diff ecash-herald-tests Build 60480: arc lint + arc unit
Event Timeline
apps/ecash-herald/src/events.js | ||
---|---|---|
214 ↗ | (On Diff #49858) | If we are relying on chronik and ts here, we can assume blockTimestamp is defined every time in handleBlockInvalidated, since the function is only called on BLK_INVALIDATED msgs which include it If we still want to handle it, then should use const timestamp = typeof blockTimestamp !== 'undefined' ? blockTimestamp: 'unknown' |
216 ↗ | (On Diff #49858) | same comment as above in this case, we are already assuming that coinbaseData is the expected shape. So we might as well assume it is also defined. |
apps/ecash-herald/src/parse.js | ||
1989–1992 ↗ | (On Diff #49858) | |
2001 ↗ | (On Diff #49858) | if we only ever call this on invalidated blocks, should never be undefined |
apps/ecash-herald/src/events.js | ||
---|---|---|
214 ↗ | (On Diff #49858) | good point, at this stage it's always defined indeed (while it's not in other handlers but we don't pass them down so it's a non issue) |
apps/ecash-herald/src/parse.js | ||
2001 ↗ | (On Diff #49858) | This is true but I'd rather not rely on this, it is brittle as opposed to the invalidated callback which offers a strong guarantee |
apps/ecash-herald/src/utils.js | ||
---|---|---|
395 | no need for this method the getCoingeckoApiUrl function is a one-off thing ... exists because I over-engineered the coingecko methods in this app to support config-adjustable prices to display. | |
405 | weird that this here is working it's ok to just define this URL as a constant, e.g. in config. | |
409 | lot of work to deal with this potential cache issue, which could also be a race condition depending on how fast blocks are coming in. I can see how it could "work" and the info would be better than nothing. We could modify chronik and chronik-client to fwd the getstakingreward RPC? Might be a useful template to support some other RPC calls going fwd. Though practically speaking ... only makes sense if the chronik URL being used is an avalanche node, which is a whole nother thing. | |
apps/ecash-herald/test/chronikWsHandler.test.js | ||
166 | we could just put the constant URL here, the use of getCoingeckoApiUrl(config) above is a weird case |
apps/ecash-herald/src/utils.js | ||
---|---|---|
409 | Chronik is an indexer, its not it's role to provide a proxy for the node RPCs |
apps/ecash-herald/src/events.js | ||
---|---|---|
255 ↗ | (On Diff #49966) | |
apps/ecash-herald/src/parse.js | ||
2054 ↗ | (On Diff #49966) | could just define it in this file, const IFP_OUTPUTSCRIPT = a914d37c4c809fe9840e7bfa77b86bd47163f6fb6c6087, so at least it is not a magic string. I don't think it is used anywhere else at the moment. |
2056 ↗ | (On Diff #49966) | in general...they're the same. But basically you should always use === unless there is a very specific case you are trying to get with == |
2068 ↗ | (On Diff #49966) | |
2083 ↗ | (On Diff #49966) | |
apps/ecash-herald/src/utils.js | ||
416 ↗ | (On Diff #49966) | is address here meant to be nextStakingReward.data.address? I don't see address anywhere earlier in this function -- so we would always expect it to be undefined here |
apps/ecash-herald/test/fixtures/invalidatedBlocks.js | ||
148 ↗ | (On Diff #49966) | it's confusing that memoryCache here is not the actual memoryCache that is passed as a param, but the result we expect to find cached at an expected block. imo expectedCacheData or similar would be less ambiguous |
apps/ecash-herald/test/mocks/block.js | ||
6105 ↗ | (On Diff #49966) | block.js is a generated file, updated by node scripts/generateMock.js if this is just a constant that is used for a test -- should be defined in a different mocks file, or just a constant in the test file that uses it. |
apps/ecash-herald/src/utils.js | ||
---|---|---|
416 ↗ | (On Diff #49966) | Oups yes and the property should be cachedObject.address... leftover from a previous iteration, good catch |
apps/ecash-herald/src/utils.js | ||
---|---|---|
415 ↗ | (On Diff #49967) | FYI in JS, this is the same const cachedObject = { scriptHex } |
apps/ecash-herald/src/utils.js | ||
---|---|---|
415 ↗ | (On Diff #49967) | TIL |