And attempt to guess the reason why it was rejected.
Details
Details
- Reviewers
bytesofman - Group Reviewers
Restricted Project - Commits
- rABC43f19a37b28f: [herald] Show invalidated blocks
npm run test
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- herald_invalidated_block
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 30440 Build 60400: Build Diff ecash-herald-tests Build 60399: 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 ↗ | (On Diff #49924) | 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 ↗ | (On Diff #49924) | weird that this here is working it's ok to just define this URL as a constant, e.g. in config. |
| 409 ↗ | (On Diff #49924) | 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 ↗ | (On Diff #49924) | 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 ↗ | (On Diff #49924) | 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 |