feat: implement emily client to get deposits and update broadcasted ones #609
Conversation
…e-broadcasted-deposits
matteojug
changed the title
…e-broadcasted-deposits
signer/src/emily_client.rs
Outdated
| // TODO: fetch multiple pages? | ||
| resp.deposits |
There was a problem hiding this comment.
Hmmm, maybe. I suppose we'll have to dig into the source of the generated deposit_api::get_deposits code to see if that is handled for us, but I cannot imagine that it is.
There was a problem hiding this comment.
The page_size value seems to be passed to the emily api endpoint, using None should mean no limit since it's used here, but at the same time there's seems to be a "forced pagination" once the resulting data reach 1MB.
signer/src/emily_client.rs
Outdated
| /// Update deposits. | ||
| fn update_broadcasted_deposits<'a>( | ||
| &'a self, | ||
| transaction: &'a UnsignedTransaction<'a>, | ||
| bitcoin_chain_tip: &'a BitcoinBlockRef, |
There was a problem hiding this comment.
I would add a little more detail here for the comment. I think this function is supposed to be used after a sweep bitcoin transaction has been confirmed, but the stacks transaction minting sBTC has not been confirmed yet. And we probably need to add in information about the stacks chain tip here too (or add in a storage parameter so that we can fetch it).
signer/src/emily_client.rs
Outdated
| // TODO: use stacks tip/hash here? | ||
| last_update_block_hash: bitcoin_chain_tip.block_hash.to_string(), | ||
| last_update_height: bitcoin_chain_tip.block_height, |
There was a problem hiding this comment.
Yeah these are stacks related.
| // TODO: need to add a retry (somewhere else?) if this fails? Otherwise | ||
| // since we already submitted the bitcoin tx, we would never mark the request | ||
| // as accepted. | ||
| self.context | ||
| .get_emily_client() |
There was a problem hiding this comment.
I don't think we need to explicitly retry here, since the fallback wrapper struct is supposed to do that for us.
There was a problem hiding this comment.
I was thinking about the case of Emily being offline or unreachable: since we already submitted the tx we would never mark it as accepted. That's why I was thinking of adding the retry in some other place, looking for inconsistencies.
I'll remove the comment though, since I guess it's the general make it robust thing.
There was a problem hiding this comment.
Realistically what we should probably do here (long-term) is store all emily "updates" in the db as a persistent queue and push them from there in a separate task. Since it's only the signer coord that sends these, and we don't know how long emily could be unreachable (could be a local network issue), that would help us avoid eating memory storing these in ram and offer better reliability in-case the signer restarts.
signer/tests/integration/emily.rs
Outdated
| context | ||
| .with_bitcoin_client(|client| { | ||
| client | ||
| .expect_estimate_fee_rate() | ||
| .once() | ||
| .returning(|| Box::pin(async { Ok(1.3) })); | ||
|
|
||
| client | ||
| .expect_get_last_fee() | ||
| .once() | ||
| .returning(|_| Box::pin(async { Ok(None) })); | ||
| }) | ||
| .await; |
There was a problem hiding this comment.
It might be helpful to add a comment about the significance of get_last_fee here, which is that it just tells the caller that they do not need to worry about replacing-by-fee another bitcoin transaction in the mempool.
signer/tests/integration/emily.rs
Outdated
| // Return the deposit tx block | ||
| client | ||
| .expect_get_block() | ||
| .once() | ||
| .returning(move |block_hash| { | ||
| let res = if *block_hash == deposit_block_hash { | ||
| Ok(Some(deposit_block.clone())) | ||
| } else { | ||
| Err(Error::MissingBlock) | ||
| }; | ||
| Box::pin(async move { res }) | ||
| }); |
There was a problem hiding this comment.
I would also note in this comment that since we've saved the deposit block hash into the database already, we do not attempt to fetch any of it's parents.
signer/tests/integration/emily.rs
Outdated
| for signer_pub_key in signer_info[0] | ||
| .signer_public_keys | ||
| .iter() | ||
| .take(signing_threshold as usize) | ||
| { |
There was a problem hiding this comment.
Can we break this out over multiple lines for readability? Something like:
let public_keys = signer_info[0]
.signer_public_keys
.iter()
.take(signing_threshold as usize);
for signer_pub_key in public_keys {
...
signer/tests/integration/emily.rs
Outdated
| /// End to end test for deposits via Emily: a deposit request is created on Emily, | ||
| /// then is picked up by the block observer, inserted into the storage and accepted. | ||
| /// After a signing round, the sweep tx for the request is broadcasted and we check | ||
| /// that Emily is informed about it. | ||
| /// | ||
| /// To run this test, concurrently run: | ||
| /// - make emily-integration-env-up | ||
| /// - AWS_ACCESS_KEY_ID=foo AWS_SECRET_ACCESS_KEY=bar AWS_REGION=us-west-2 make emily-server | ||
| /// then, once Emily is up and running, run this test. | ||
| #[ignore = "This is an integration test that requires manually running emily"] | ||
| #[tokio::test] | ||
| async fn deposit_e2e() { |
Description
Closes: #602, #605
Changes
Implement the EmilyClient to get and update deposits status. And add an integration test for the Emily flow.
Testing Information
Added an integration test, to be run manually, to test the e2e flow wrt Emily: we generate the deposit request there, it's picked up by the block observer, then we accept it and trigger the sweep tx; finally we check that request status on Emily is updated to accepted. The test passes.
Checklist: