T3060
Depends on D13633
Check isFinalBlock status. If block is not final, do not update aliases in handleBlockConnected.
Details
Details
- Reviewers
Fabien - Group Reviewers
Restricted Project - Commits
- rABCc0a66d183fd1: [alias-server] Check avalanche isFinalBlock in handleBlockConnected
npm test
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 23198 Build 46015: Build Diff Build 46014: arc lint + arc unit
Event Timeline
Comment Actions
Why don't you use the same strategy as chronik here to avoid axios ? Create a node object from the rpc url and some secrets (user/password), and pass it done the stack. This lets you create a mocked node that implements isFinalBlock and return whatever you want.
| apps/alias-server/index.js | ||
|---|---|---|
| 92 ↗ | (On Diff #39434) | Why is that a secret ? |
| apps/alias-server/src/events.js | ||
| 113 ↗ | (On Diff #39434) | |
Comment Actions
it would be nice to remove a dependency, but axios is pretty useful here. without it, would need to add logic for parsing server response, converting to json. Then also make a mock for that.
axios is pretty standard for API calls in nodejs apps. might be used for other API calls in this app as well (e.g. XEC price).
| apps/alias-server/index.js | ||
|---|---|---|
| 92 ↗ | (On Diff #39434) | includes rpcusername and rpcpassword |
Comment Actions
handle this todo: TODO if not finalized, keep checking at set intervals until a given timeout
| apps/alias-server/src/events.js | ||
|---|---|---|
| 101 | If you have access to config, why don't you use it for avalancheCheckWaitInterval as well ? | |
| apps/alias-server/src/events.js | ||
|---|---|---|
| 101 | avalancheCheckWaitInterval needs to be a param, so that it can be zero in the unit tests Though I guess with this logic, that is no longer the case. I think it's worth keeping the param though since, in practical testing, I was never getting isFinalBlock until after about 24 seconds. So we should probably keep an initial "wait(passedParameter)" that is set to zero in the unit tests. I expect there will need to be changes to this logic based on lessons learned in live testing, and the passed param will be useful at that point. | |
| apps/alias-server/src/events.js | ||
|---|---|---|
| 101 | I don't understand your results, they are very different from why I get on my nodes. I suppose it depends on the machine but also the delays from the the ws messages/RPC themselves. But the point is that you can avoid that logic entirely for the tests by doing the call first, then wait. I see no benefit in doing the opposite: it's more code and less efficient. | |
Comment Actions
Improving log statement
| apps/alias-server/src/events.js | ||
|---|---|---|
| 98 ↗ | (On Diff #39562) | In this case it could be ++ I always use +=1 because there are slight differences in how return values are handled with ++, i.e. ++i and i++ mean different things in different contexts in JS linter doesn't seem to have a problem either way. I don't have a strong personal attachment to +=1, just kind of the way I've always done it. +=1 is used everywhere else in the app so leaving it for now. If we want ++ to be the standard, should use a linter rule. |
| apps/alias-server/src/events.js | ||
|---|---|---|
| 98 ↗ | (On Diff #39562) | OK, thanks for the explanation. Nothing wrong here, it's just an uncommon pattern to use +=1 in loops (at least it is for me). |