[2/n][guardian-integration] in-process e2e harness#466
Conversation
aac0a18 to
2fd7fb1
Compare
2fd7fb1 to
f58edae
Compare
370b474 to
942504b
Compare
f58edae to
c124d70
Compare
942504b to
91bcf58
Compare
c124d70 to
2fcf91f
Compare
91bcf58 to
e0a17e2
Compare
2fcf91f to
56510fb
Compare
e0a17e2 to
1422d8d
Compare
56510fb to
1bb33df
Compare
1422d8d to
89064c8
Compare
96f3774 to
49100e6
Compare
| aws-sdk-s3 = "1.12.0" | ||
| aws-credential-types = "1" | ||
|
|
||
| aws-smithy-mocks = { version = "0.1", optional = true } |
There was a problem hiding this comment.
nit: any way to remove the mock library from the main dependency list? IIUC it is just used by the test harness, right?
There was a problem hiding this comment.
Yeah, afaik we can remove it, but then we have to pass --features test-utils everytime we want to run a test.
but i think we might be able to manage that via nextest profiles.
let me check.
f5289b5 to
67c9469
Compare
bmwill
left a comment
There was a problem hiding this comment.
not major issues, mostly just some nits or asks to add TODOs so we don't forget things.
| run_bitcoin_withdrawal_e2e(true).await | ||
| } | ||
|
|
||
| async fn run_bitcoin_withdrawal_e2e(with_guardian: bool) -> Result<()> { |
There was a problem hiding this comment.
I suppose this makes sense while we are getting the integration hooked up. We may want to evaluate if we want to have all tests run with guardian by default eventually.
| /// Start an operator-init'd guardian. Withdrawal RPCs stay gated until | ||
| /// [`Self::finalize`] completes provisioner-init. | ||
| pub async fn start(network: Network) -> Result<Self> { |
There was a problem hiding this comment.
At which stage is the guardian pubkey available? Here or after finalize? we do need to commit to it, and likely a url of some such, onchain and it would probably be beneficial to be able to have the key available at contract init time.
There was a problem hiding this comment.
From a conversation we had today, the committing onchain will be done post this series
| .await | ||
| .context("bind guardian harness listener")?; | ||
| let addr: SocketAddr = listener.local_addr()?; | ||
| let endpoint = format!("http://{addr}"); |
There was a problem hiding this comment.
Just a note, do we want to use traditional TLS termination or require mTLS from the validators to the guardian?
| nodes[0] | ||
| .wait_for_mpc_key(std::time::Duration::from_secs(120)) | ||
| .await?; |
There was a problem hiding this comment.
We do we need this if we fetch the key from onchain below?
| .onchain_state() | ||
| .current_committee() | ||
| .ok_or_else(|| anyhow::anyhow!("no current committee after DKG"))?; | ||
| let master_pubkey = hashi.get_onchain_mpc_pubkey()?; |
| /// URL of the `hashi-guardian` gRPC endpoint. When not set, the guardian | ||
| /// integration is bypassed. | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub guardian_endpoint: Option<String>, |
There was a problem hiding this comment.
maybe add a TODO that this is temporary and will be removed once we commit it onchain?
| pub struct GuardianClient { | ||
| endpoint: String, | ||
| channel: Channel, | ||
| } |
There was a problem hiding this comment.
We should instrument this with metrics like we do our other gRPC client. Happy to add a TODO and add them as a follow up PR.
| /// TODO: Investigate if it can be moved to std::sync::Mutex | ||
| pub shares: tokio::sync::Mutex<Vec<Share>>, |
There was a problem hiding this comment.
yeah we should have this be std::sync::Mutex
| pub mod enclave; | ||
| pub mod getters; | ||
| pub mod heartbeat; | ||
| pub mod init; | ||
| pub mod rpc; | ||
| pub mod s3_logger; // used by the monitor | ||
| pub mod setup; | ||
| pub mod withdraw; |
There was a problem hiding this comment.
were these files just not getting compiled?
| mod getters; | ||
| mod heartbeat; | ||
| mod init; | ||
| mod rpc; | ||
| mod s3_logger; | ||
| mod setup; | ||
| mod withdraw; |
There was a problem hiding this comment.
ah this is where they were.
`GuardianHarness::finalize` and `create_fully_initialized_enclave` were running the same "drive an operator-init'd enclave to fully-initialized state" sequence (set BTC keys, build init state, state.init, mark logged) inline. Lift it into `hashi_guardian::test_utils::finalize_enclave` and call it from both. The harness's `finalize` shrinks to a single helper call plus the `is_fully_initialized` post-condition. No visibility or API changes to the `Enclave` types.
The `network` field was set in `start()` but never read internally; the getter was marked `#[allow(dead_code)]`. The enclave already owns the network, and `pub` already opts out of dead-code lint, so the attribute on a `pub` API was a smell.
Extract `run_bitcoin_withdrawal_e2e(with_guardian: bool)` as the shared body for the deposit → withdrawal → confirm flow. Both `test_bitcoin_withdrawal_e2e_flow` and `test_bitcoin_withdrawal_with_guardian_e2e_flow` now reduce to one-line wrappers that pass `false` / `true`. Forces the two flows to stay in lockstep — any future change to the deposit/withdrawal path is automatically exercised under both configurations. The guardian variant gains the post-deposit balance assertion and the withdrawal_request_id / withdrawal_txid log lines that were previously only in the no-guardian variant; behavior is otherwise unchanged. `setup_test_networks` is no longer called from the bitcoin-withdrawal test (its setup is now inlined into the parameterized helper) but remains in use by the other 3 e2e tests.
Reuse setup_test_networks via a shared `_with(builder)` helper so the guardian withdrawal flow doesn't re-implement the network-init logging. Trim doc comments on guardian_harness, finalize_enclave, and create_fully_initialized_enclave.
Collapse the wrapper + helper pair into one function that always takes a builder. The 3 existing call sites pass `TestNetworksBuilder::new() .with_nodes(4)` explicitly, which removes the wrapper-of-a-wrapper and keeps any future setup customization at the call site.
67c9469 to
dffff49
Compare
Summary
Enclave(+EnclaveConfig/EnclaveState/Scratchpad) frommain.rsinto a newenclave.rslib module— @mskd12 this is just a simple move. no changes made to the code.
enclave_btc_keypairandhashi_btc_master_pubkeywent from private topub(crate).init.rs::testsreads these fields directly viaenclave.config.<field>.get()enclave.rsallows the e2e-tests to construct and derive an enclave in-processtest-utilsfeature onhashi-guardianexposing constructors for partially/fully-initialized enclaves.Tests harness changes
e2e-tests::GuardianHarnessruns an in-process guardian over real gRPC.TestNetworksBuilder::with_guardian()orchestrates start → DKG → finalize.test_bitcoin_withdrawal_with_guardian_e2e_flowdoes full e2e flow with guardian