Page MenuHomePhabricator

[alias-server] Pass aliasConstants as param with single appSettings object
AbandonedPublic

Authored by bytesofman on Jul 3 2023, 22:57.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Summary

T3060

Remove technical debt in alias-server. Already juggling a lot of app params here. Adding one more is not feasible without using a single object.

Use appSettings to pass server params through relevant functions.

Pass aliasConstants as a param instead of pulling it into functions.

Test Plan

npm test, node index.js and app indexes through latest block

Diff Detail

Repository
rABC Bitcoin ABC
Branch
alias-server-use-constants-as-param
Lint
Lint Errors
SeverityLocationCodeMessage
Errorapps/alias-server/src/chronikWsHandler.js:35ESLINTno-unused-vars
Errorapps/alias-server/src/chronikWsHandler.js:36ESLINTno-unused-vars
Errorapps/alias-server/src/chronikWsHandler.js:37ESLINTno-unused-vars
Errorapps/alias-server/src/chronikWsHandler.js:38ESLINTno-unused-vars
Errorapps/alias-server/src/chronikWsHandler.js:39ESLINTno-unused-vars
Errorapps/alias-server/src/chronikWsHandler.js:40ESLINTno-unused-vars
Errorapps/alias-server/test/mainTests.js:71ESLINTno-unused-vars
Errorapps/alias-server/test/mainTests.js:72ESLINTno-unused-vars
Unit
No Test Coverage
Build Status
Buildable 24330
Build 48269: Build Diffalias-server-tests
Build 48268: arc lint + arc unit

Event Timeline

pass as a param to startServer

Fabien requested changes to this revision.Jul 4 2023, 07:12
Fabien added a subscriber: Fabien.

What is the plan after this diff ? Will you add more params to the object ?

As of now I don't see the benefit of this changes. Looking at the params:

  • db is obviously something you want to pass down the app, same for chronik. Telegram bot is not necessary for the alias server to run, so having it as a param makes sense so you can disable it as needed.
  • the alias constants are constants, right ? So there is no point passing them down
  • the channel ID should be a member of the telegram bot, it's the only user of this data
  • the avalanche RPC is just a bandaid because you're not using the new chronik version that already has everything you need to make this faster and more robust without the need for the RPC at all

So by just improving on the code you can get rid of half the parameters. That looks like a much better approach than obfuscating the params, giving a false feeling of simplification but eventuallty making it harder to figure out what's wrong and needs a refactor later.

This revision now requires changes to proceed.Jul 4 2023, 07:12

hm good points

will look at this when tg bot is implemented for combining those.