Dynamic node list - #474
Conversation
|
Thanks for the update 🙏🏼 . To make the implementation easier for the team to maintain in the future, I suggest moving this logic into the existing services:
Keeping the logic inside the existing service structure should make the behavior clearer and easier to test and maintain. This is the suggested direction rather than a required implementation. Please feel free to propose another approach that keeps node-loading responsibility within nodes.service.js. |
| } | ||
| } | ||
|
|
||
| function loadNetworkNodes(key, url) { |
There was a problem hiding this comment.
You can move this logic into nodes.service.js.
| } | ||
|
|
||
| Promise.all([ | ||
| loadNetworkNodes('mainnet', NODEWATCH_URLS.mainnet).then(function (n) { freshByNetwork.mainnet = n; }), |
There was a problem hiding this comment.
This method can be used in the connect() method in dataBridge.service.js, since this is where the wallet begins selecting a node and establishing its connection.
| <!-- Dynamic node list (nodewatch.symbol.tools; falls back to bundled defaults on any failure) --> | ||
| <script src="vendors/dynamic-nodes.js"></script> | ||
|
|
There was a problem hiding this comment.
Once it moves into existing services, we no longer need it here.
e409edf to
5a2776e
Compare
|
Thanks for the review. |
| import nem from 'nem-sdk'; | ||
| import UrlParser from 'url-parse'; | ||
|
|
||
| var NODEWATCH_URLS = { |
There was a problem hiding this comment.
Since both the URL and cache key are network-specific, could we centralize them in a network configuration map? For example:
const NODEWATCH_BASE_URL = 'https://nodewatch.symbol.tools';
const NETWORK_NODE_CONFIG = {
[nem.model.network.data.mainnet.id]: {
url: `${NODEWATCH_BASE_URL}/api/nem/nodes`,
cacheKey: 'nodeWatchNodes_mainnet'
},
[nem.model.network.data.testnet.id]: {
url: `${NODEWATCH_BASE_URL}/testnet/api/nem/nodes`,
cacheKey: 'nodeWatchNodes_testnet'
}
};
This keeps the related configuration together and removes the need to derive the network name and cache key separately.
What do you think?
| mainnet: 'https://nodewatch.symbol.tools/api/nem/nodes', | ||
| testnet: 'https://nodewatch.symbol.tools/testnet/api/nem/nodes' | ||
| }; | ||
| var DYNAMIC_NODES_DEFAULT_PORT = 7890; |
There was a problem hiding this comment.
Instead of defining another default-port constant, could we reuse the value already provided by nem-sdk?
nem.model.nodes.defaultPort contains the same port (7890) and is already used elsewhere in the wallet, so reusing it would avoid duplicating this value.
| var DYNAMIC_NODES_HEIGHT_TOLERANCE = 20; | ||
| var DYNAMIC_NODES_MIN_NODES = 3; | ||
| var DYNAMIC_NODES_FETCH_TIMEOUT_MS = 8000; | ||
| var DYNAMIC_NODES_CACHE_TTL_MS = 6 * 60 * 60 * 1000; | ||
| var DYNAMIC_NODES_PRIVATE_HOST_RE = /^(127\.|10\.|172\.(1[6-9]|2\d|3[01])\.|192\.168\.|169\.254\.|0\.0\.0\.0$)|^localhost$|^\[?::1\]?$/i; |
There was a problem hiding this comment.
Could we replace the generic DYNAMIC_NODES_* prefix with NODEWATCH_NODES_* or simply NODEWATCH_*? This makes it clearer that these constants specifically configure the node list retrieved from NodeWatch.
2 participants
No description provided.