Conversation
|
👋 Hi! Please choose at least one reviewer by assigning them on the right bar. |
7303c74 to
354137f
Compare
354137f to
0f7bc37
Compare
| ChainSource::Rpc { rpc_address, rpc_user, rpc_password } => { | ||
| builder.set_chain_source_bitcoind_rpc( | ||
| rpc_address.ip().to_string(), | ||
| rpc_address.port(), | ||
| rpc_user, | ||
| rpc_password, | ||
| ); | ||
| ChainSource::Rpc { rpc_host, rpc_port, rpc_user, rpc_password } => { | ||
| builder.set_chain_source_bitcoind_rpc(rpc_host, rpc_port, rpc_user, rpc_password); |
There was a problem hiding this comment.
Up to you people to make the call, I'd prefer parsing a single <hostname>:<port> string rather than splitting that setting into two parameters. This is what I do in VSS, it optimizes for fewer config params. That setting is logically a single thing right?
There was a problem hiding this comment.
That setting is logically a single thing right?
I agree it is. The problem was that we parse this setting using SocketAddr and that has a limitation of only supporting IP addresses. In containerized environments where the IP address might change if the container restarts, the user will have to update this setting each time it restarts. But supporting hostname simplifies things, because you only have to map the setting to a hostname and not worry if/when the IP changes.
An alternative to using SocketAddr (without splitting) is the ToSocketAddrs trait which supports hostname but it's sync and blocking.
UPDATE:
Missed "I'd prefer parsing a single <hostname>:<port>" from the comment earlier. Will drop the split.
This splits the
rpc_addressconfiguration field into two fields,rpc_hostandrpc_portto allow hostname support, especially in containerized environments.Closes #66