Page MenuHomePhabricator

[alias-server] Check avalanche isFinalBlock in handleBlockConnected
ClosedPublic

Authored by bytesofman on Apr 8 2023, 14:08.

Details

Summary

T3060

Depends on D13633

Check isFinalBlock status. If block is not final, do not update aliases in handleBlockConnected.

Test Plan

npm test

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 23211
Build 46041: Build Diff
Build 46040: arc lint + arc unit

Event Timeline

Fabien requested changes to this revision.Apr 9 2023, 09:13
Fabien added a subscriber: Fabien.

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)
This revision now requires changes to proceed.Apr 9 2023, 09:13

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.

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

handle this todo: TODO if not finalized, keep checking at set intervals until a given timeout

handle this todo: TODO if not finalized, keep checking at set intervals until a given timeout

See comment in D13633

this can be added later if needed

This revision is now accepted and ready to land.Apr 10 2023, 19:11
Fabien requested changes to this revision.Apr 11 2023, 04:49

You didn't change the logic at all

This revision now requires changes to proceed.Apr 11 2023, 04:49
Fabien requested changes to this revision.Apr 11 2023, 14:21
Fabien added inline comments.
apps/alias-server/src/events.js
101 ↗(On Diff #39541)

If you have access to config, why don't you use it for avalancheCheckWaitInterval as well ?

This revision now requires changes to proceed.Apr 11 2023, 14:21
bytesofman added inline comments.
apps/alias-server/src/events.js
101 ↗(On Diff #39541)

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.

Fabien requested changes to this revision.Apr 11 2023, 16:29
Fabien added inline comments.
apps/alias-server/src/events.js
101 ↗(On Diff #39541)

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.

This revision now requires changes to proceed.Apr 11 2023, 16:29

Get avalancheCheckWaitInterval from config, not param

Fabien requested changes to this revision.Apr 12 2023, 09:03

Couple nits but it's getting there

apps/alias-server/src/events.js
98

is there no ++ operator in javascript ?

117
This revision now requires changes to proceed.Apr 12 2023, 09:03
bytesofman marked an inline comment as done.

Improving log statement

apps/alias-server/src/events.js
98

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.

Fabien added inline comments.
apps/alias-server/src/events.js
98

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).

This revision is now accepted and ready to land.Apr 12 2023, 14:02