Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(agents): improve agent config #171
feat(agents): improve agent config #171
Changes from 14 commits
75dcc18
31a3018
e20058c
69492e6
488be75
4c8f62b
c437483
9d6897c
203a5c7
d9de4c0
7b7aae0
9d7c644
b262c00
ae9cb60
c24918f
cfb2492
ed655d7
ef1fd52
b45b6ed
75d5ef6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we currently use the default option no matter what, maybe we can have
SignerConf::from_env
andChainConf::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 varThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think they should be kept prefix-agnostic. You can call
SignerConf::from_env
a second time with the default prefix butChainConf::from_env
needs to be able to handle two prefixes (*_CONNECTION_URL
has no default). Passing prefix and default prefix separately givesFromEnv
implementers the required flexibility.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you're right about this. Smells to me for some reason but also simplifies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 paramThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 anOption<&str>
is fine.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expected precedence from lowest to highest:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 intest-signer-default
test fixture.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And on that note we need to DRY up agent settings tests... another chore lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. Might have some q's