feat(agents): improve agent config #171
Conversation
This reverts commit b262c00.
| let chain_conf = | ||
| ChainConf::from_env(&format!("RPCS_{}", network_upper), Some("RPCS_DEFAULT"))?; | ||
|
|
||
| let transaction_signer = SignerConf::from_env( | ||
| &format!("TRANSACTIONSIGNERS_{}", network_upper), | ||
| Some("TRANSACTIONSIGNERS_DEFAULT"), | ||
| )?; |
There was a problem hiding this comment.
Since we currently use the default option no matter what, maybe we can have SignerConf::from_env and ChainConf::from_env look for DEFAULT in env vars rather than passing in arg? As in always look first for DEFAULT, if not there then assume network-specific var
There was a problem hiding this comment.
I think they should be kept prefix-agnostic. You can call SignerConf::from_env a second time with the default prefix but ChainConf::from_env needs to be able to handle two prefixes (*_CONNECTION_URL has no default). Passing prefix and default prefix separately gives FromEnv implementers the required flexibility.
There was a problem hiding this comment.
Maybe you're right about this. Smells to me for some reason but also simplifies.
There was a problem hiding this comment.
The other option is to separate prefix and network and have network be an optional param. You can first check DEFAULT_{prefix} then check {network}_{prefix} or {prefix}_{network} this way. Think this may be more intuitive than using a hardcoded static default string as param
There was a problem hiding this comment.
Actually, I'm going to stick with the original. I think it's ok for an impl to have hardcoded keys (makes little sense to pass everything in) but the interface should indicate that a default is going to be checked. Passing in only network/prefix and then having it pull a default value is too much magic imo. The alternative would be a check_default param or sth similar but I think an Option<&str> is fine.
There was a problem hiding this comment.
Expected precedence from lowest to highest:
- TRANSACTIONSIGNERS_DEFAULT
- TRANSACTIONSIGNERS_RINKEBY
|
|
||
| #[test] | ||
| #[serial_test::serial] | ||
| fn it_builds_from_env_default() { |
There was a problem hiding this comment.
Could we add a default test case to kathy/src/settings.rs? We've been using kathy settings as place for testing full e2e configuration of agent from env to agent settings block. We'd just need to essentially copy this test except piping in test-signer-default test fixture.
There was a problem hiding this comment.
And on that note we need to DRY up agent settings tests... another chore lol
There was a problem hiding this comment.
Sounds good. Might have some q's
|
FYI, I've set Should not have a negative impact anywhere wrt correctness / security. Ref https://doc.rust-lang.org/cargo/reference/features.html#feature-resolver-version-2 |
|
Tested in dev with Goerli network and it works as expected. |
|
Great, thanks @arnaud036 |
lattejed
force-pushed
the
lattejed/agent_config
branch
from
f63297e to
75d5ef6
Compare
Motivation
Agent config should be improved to speed up deployments. Currently most config changes have to go through a PR->deploy cycle. Configs should be updatable after deployment
All necessary settings should be overridable with env varsRef #169
[Edit]
Going to cap this PR here since the changes already made allow deploys to be simplified / sped up. Further improvements to agent config can be done later.
Solution
A number of related items need to be done:
PR Checklist