Skip to content
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

feature/refactor: integrate NomadConfig #30

Merged
merged 37 commits into from
Mar 23, 2022
Merged

Conversation

luketchang
Copy link
Collaborator

@luketchang luketchang commented Mar 16, 2022

Changes

  • NomadIdentifier and NameOrDomain moved to new nomad-types crate
  • Configuration crate moved into rust repo
  • All pure configuration structs/enums moved to configuration crate (e.g. SignerConf, ChainConf)
  • Agent-specific settings are mix of base settings + agent settings from configuration crate
  • Adds AgentsSecrets struct which is deserialized from secrets.json
  • Full base + agent-specific settings built from NomadConfig + AgentSecrets
  • Paging settings made per-chain not single for entire agent

Chores

  • All base and agent-specific configurations unit tested
  • Agents run locally to ensure proper bootup

Closes #28, #31, #14

@luketchang luketchang changed the title WIP: integrate NomadConfig feature/refactor: integrate NomadConfig Mar 20, 2022
@luketchang luketchang marked this pull request as ready for review March 20, 2022 22:39
@@ -14,5 +15,5 @@ members = [
"agents/processor",
"tools/kms-cli",
"tools/nomad-cli",
"tools/balance-exporter"
# "tools/balance-exporter"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will delete in separate PR: #38

Comment on lines +9 to +45
mod test {
use super::*;
use nomad_base::{AgentSecrets, NomadAgent};

const RUN_ENV: &str = "test";
const AGENT_HOME: &str = "ethereum";
const SECRETS_PATH: &str = "../../fixtures/secrets.json";

#[test]
fn it_builds_settings_from_config_and_secrets() {
std::env::set_var("RUN_ENV", RUN_ENV);
std::env::set_var("AGENT_HOME", AGENT_HOME);
std::env::set_var("SECRETS_PATH", SECRETS_PATH);

decl_settings!(Processor {
/// The polling interval (in seconds)
interval: String,
/// An allow list of message senders
allowed: Option<HashSet<H256>>,
/// A deny list of message senders
denied: Option<HashSet<H256>>,
/// Only index transactions if this key is set
indexon: Option<String>,
/// An amazon aws s3 bucket to push proofs to
s3: Option<S3Config>,
});
let settings = ProcessorSettings::new().unwrap();

let config = nomad_xyz_configuration::get_builtin(RUN_ENV).unwrap();
let secrets = AgentSecrets::from_file(SECRETS_PATH).unwrap();

settings
.base
.validate_against_config_and_secrets(
crate::Processor::AGENT_NAME,
AGENT_HOME,
config,
&secrets,
)
.unwrap();

let agent_config = &config.agent().get("ethereum").unwrap().processor;
assert_eq!(settings.agent.interval, agent_config.interval);
assert_eq!(settings.agent.allowed, agent_config.allowed);
assert_eq!(settings.agent.denied, agent_config.denied);
assert_eq!(settings.agent.index_only, agent_config.index_only);
assert_eq!(settings.agent.s3, agent_config.s3);
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have issue up to DRY up agent settings tests: #34

@@ -0,0 +1,138 @@
export type NomadIdentifier = string;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to update definitions. Leaving for later PR due to avoid adding more scope to already overloaded PR: #39

/// # Arguments
///
/// * `s` - The hex str
pub fn strip_0x_prefix(s: &str) -> &str {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#32

@prestwich prestwich marked this pull request as ready for review March 23, 2022 16:04
pub attestation_signer: Option<SignerConf>,
}

impl AgentSecrets {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

open an issue for from_env

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#41

@@ -11,6 +11,8 @@ edition = "2021"
serde = "1.0.120"
serde_json = { version = "1.0.61", default-features = false }
ethers = {git = "https://github.com/gakonst/ethers-rs", branch = "master", features = ["abigen"]}
ethers-core = {git = "https://github.com/gakonst/ethers-rs", branch = "master"}
ethers-providers = {git = "https://github.com/gakonst/ethers-rs", branch = "master"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

open an issue for simplifiying our ethers dependencies 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#42

}

#[allow(dead_code)]
// use nomad_base::{decl_settings, NomadAgent};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe just delete the file at this point. no need for it

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#38

@@ -46,12 +48,14 @@ where

impl<I> ContractSync<I> {
/// Instantiate new ContractSync
#[allow(clippy::too_many_arguments)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ouch 💀 put this on our tech debt plate

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#44

.and_then(|s| s.parse::<u32>().ok())
.unwrap_or(1999)
/// Get agent-specific index settings unique to that agent
pub fn from_agent_name(agent_name: &str) -> Self {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

future: this should be type-associated information on the agent type. open an issue here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#43

@@ -221,39 +153,47 @@ pub struct Settings {
/// The path to use for the DB file
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the longterm goal should be to deprecate and remove this type in favor of using the config types more directly

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep I think having a struct BaseAgentSettings in configuration would be good and then all the types with lots of functionality (e.g. CachingHome) implement From<BaseAgentSettings>. Something like that where instead of implementing into_<some struct> we implement from_<some config struct>

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#40

@@ -25,6 +25,12 @@ rocksdb = { git = "https://github.com/rust-rocksdb/rust-rocksdb" }
prometheus = "0.12.0"
bytes = { version = "1", features = ["serde"]}
num = {version="0", features=["serde"]}
rusoto_core = "0.47.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like moving the aws deps into core 🤔 where's a better place to put these in the future?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah only reason this went here is so we could build a Signers from SignerConf. Best solution is probably to keep SignerExt in nomad-core and move Signers (enum over Aws and Local types) into nomad-base

Copy link
Collaborator Author

@luketchang luketchang Mar 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well turns out ethers technically imports rusoto_core anyways thru AwsSigner types. So regardless, aws stuff has to be a dependency of nomad-core if we import ethers

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack oh well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support new NomadConfig from nomad-xyz/configuration
2 participants