T3060
Depends on D13631
App should wait ~10s before checking if a found block is avalanche finalized. We don't want to do this in unit tests.
So, make this a parameter and pass to all required functions.
Details
Details
- Reviewers
Fabien - Group Reviewers
Restricted Project - Commits
- rABCa1dd51828f9c: [alias-server] Add avalancheCheckWaitInterval as a parameter to prevent 10s…
npm test
npm test
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 23142 Build 45904: Build Diff Build 45903: arc lint + arc unit
Event Timeline
| apps/alias-server/config.js | ||
|---|---|---|
| 8 ↗ | (On Diff #39433) | |
| apps/alias-server/index.js | ||
| 84 ↗ | (On Diff #39433) | You might as well just pass config to main and all the way down, and avoid adding more and more options over time |
| apps/alias-server/src/events.js | ||
| 91 ↗ | (On Diff #39433) | This is not an efficient strategy, you should rather retry in a loop with a small delay, up to a timeout. Pseudo code: var timeout = 10000;
while (!avalanche_finalized(blockhash) && timeout >= 0) {
wait(500);
timeout -= 500;
} |
| 92 ↗ | (On Diff #39433) | No blocker, but it would be easier to read if that was a made a generic function, like waitAsync(delay) or something like that |
| apps/alias-server/index.js | ||
|---|---|---|
| 84 ↗ | (On Diff #39433) | Yes. I think its best done after this stack though, so that there are existing unit tests on main() |
| apps/alias-server/src/events.js | ||
| 93 ↗ | (On Diff #39471) | this is more readable. i'm not sure how to unit test the function, it's just a JS primitive |
| 91 ↗ | (On Diff #39433) | ok, but this should be done in the function that checks for isFinalBlock, as the value is needed for this loop in this diff we only introduce the parameter. Updated the todo |
| apps/alias-server/src/events.js | ||
|---|---|---|
| 91 ↗ | (On Diff #39433) | While this approach is better, imo this is another feature that may not be needed and is not relevant to this stack The right way to fix this is to have chronik send block connected msgs on avalanche finalized blocks, obviating the need for rpc entirely. This type of "keep checking for avalanche status" functionality can be added as an optimization if we are seeing this issue in prod. |
| apps/alias-server/src/events.js | ||
|---|---|---|
| 93 ↗ | (On Diff #39471) | You don't need to unit test this function, there is no logic there |
| 91 ↗ | (On Diff #39433) |
Agreed. |
| 91 ↗ | (On Diff #39433) | I disagree. It's not another way to do it, it's the proper way to do it. Look at it from another angle: you stacked a diff to bypass this delay during the tests. With the suggested method, you don't need it at all. That's an immediate win. |
| apps/alias-server/src/events.js | ||
|---|---|---|
| 94 ↗ | (On Diff #39518) | Note: testing this over 20 blocks Max time to avalanche final block: 41 seconds So -- even with a high-ish resolution of "check every half second" -- we should probably wait 20s before checks begin. |
| apps/alias-server/src/events.js | ||
|---|---|---|
| 95 ↗ | (On Diff #39518) | Check first, then wait only if needed. Even if that's never reached in real world that's what you want for the tests. |
| apps/alias-server/src/events.js | ||
|---|---|---|
| 95–96 ↗ | (On Diff #39540) | |