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

[chore] Reject duplicate deposit requests when first one already accepted | confirmed | failed #1179

Merged
merged 11 commits into from
Jan 9, 2025
Original file line number Diff line number Diff line change
Expand Up @@ -510,6 +510,16 @@
}
}
},
"409": {
"description": "Duplicate request",
"content": {
"application/json": {
"schema": {
"$ref": "#/components/schemas/ErrorResponse"
}
}
}
},
"500": {
"description": "Internal server error",
"content": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -429,6 +429,16 @@
}
}
},
"409": {
"description": "Duplicate request",
"content": {
"application/json": {
"schema": {
"$ref": "#/components/schemas/ErrorResponse"
}
}
}
},
"500": {
"description": "Internal server error",
"content": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -510,6 +510,16 @@
}
}
},
"409": {
"description": "Duplicate request",
"content": {
"application/json": {
"schema": {
"$ref": "#/components/schemas/ErrorResponse"
}
}
}
},
"500": {
"description": "Internal server error",
"content": {
Expand Down
21 changes: 18 additions & 3 deletions emily/handler/src/api/handlers/deposit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,6 @@ use stacks_common::codec::StacksMessageCodec as _;
use tracing::{debug, instrument};
use warp::reply::{json, with_status, Reply};

use bitcoin::ScriptBuf;
use warp::http::StatusCode;

use crate::api::models::deposit::{Deposit, DepositInfo};
use crate::api::models::{
deposit::requests::{
Expand All @@ -26,6 +23,8 @@ use crate::database::entries::deposit::{
DepositEntry, DepositEntryKey, DepositEvent, DepositParametersEntry,
ValidatedUpdateDepositsRequest,
};
use bitcoin::ScriptBuf;
use warp::http::StatusCode;

/// Get deposit handler.
#[utoipa::path(
Expand Down Expand Up @@ -199,6 +198,7 @@ pub async fn get_deposits(
(status = 400, description = "Invalid request body", body = ErrorResponse),
(status = 404, description = "Address not found", body = ErrorResponse),
(status = 405, description = "Method not allowed", body = ErrorResponse),
(status = 409, description = "Duplicate request", body = ErrorResponse),
(status = 500, description = "Internal server error", body = ErrorResponse)
)
)]
Expand All @@ -213,6 +213,21 @@ pub async fn create_deposit(
context: EmilyContext,
body: CreateDepositRequestBody,
) -> Result<impl warp::reply::Reply, Error> {
// Reject if we already have a deposit with the same txid and output index and it is NOT pending or reprocessing.
let entry = accessors::get_deposit_entry(
&context,
&DepositEntryKey {
bitcoin_txid: body.bitcoin_txid.clone(),
bitcoin_tx_output_index: body.bitcoin_tx_output_index,
},
)
.await;
matteojug marked this conversation as resolved.
Show resolved Hide resolved
if let Ok(deposit) = entry {
if deposit.status != Status::Pending && deposit.status != Status::Reprocessing {
return Err(Error::Conflict);
}
}

// Set variables.
let api_state = accessors::get_api_state(&context).await?;
api_state.error_if_reorganizing()?;
Expand Down
126 changes: 126 additions & 0 deletions emily/handler/tests/integration/deposit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -548,3 +548,129 @@ async fn update_deposits_updates_chainstate() {
);
}
}

#[cfg_attr(not(feature = "integration-tests"), ignore)]
#[tokio::test]
async fn overwrite_deposit() {
matteojug marked this conversation as resolved.
Show resolved Hide resolved
async fn test_duplicate(status: Status, should_reject: bool) {
let configuration = clean_setup().await;

// Arrange.
// --------
let bitcoin_txid: &str = "bitcoin_txid_overwrite_deposit";
let bitcoin_tx_output_index = 0;

// Setup test deposit transaction.
let DepositTxnData {
reclaim_script, deposit_script, ..
} = DepositTxnData::new(DEPOSIT_LOCK_TIME, DEPOSIT_MAX_FEE, DEPOSIT_AMOUNT_SATS);

let create_deposit_body = CreateDepositRequestBody {
bitcoin_tx_output_index,
bitcoin_txid: bitcoin_txid.into(),
deposit_script: deposit_script.clone(),
reclaim_script: reclaim_script.clone(),
};

apis::deposit_api::create_deposit(&configuration, create_deposit_body.clone())
.await
.expect("Received an error after making a valid create deposit request api call.");

let response = apis::deposit_api::get_deposit(
&configuration,
bitcoin_txid,
&bitcoin_tx_output_index.to_string(),
)
.await
.expect("Received an error after making a valid get deposit api call.");
assert_eq!(response.bitcoin_txid, bitcoin_txid);
assert_eq!(response.status, Status::Pending);

let mut fulfillment: Option<Option<Box<Fulfillment>>> = None;

if status == Status::Confirmed {
fulfillment = Some(Some(Box::new(Fulfillment {
bitcoin_block_hash: "bitcoin_block_hash".to_string(),
bitcoin_block_height: 23,
bitcoin_tx_index: 45,
bitcoin_txid: "test_fulfillment_bitcoin_txid".to_string(),
btc_fee: 2314,
stacks_txid: "test_fulfillment_stacks_txid".to_string(),
})));
}

apis::deposit_api::update_deposits(
&configuration,
UpdateDepositsRequestBody {
deposits: vec![DepositUpdate {
bitcoin_tx_output_index: bitcoin_tx_output_index,
bitcoin_txid: bitcoin_txid.into(),
fulfillment,
last_update_block_hash: "update_block_hash".into(),
last_update_height: 34,
status,
status_message: "foo".into(),
}],
},
)
.await
.expect("Received an error after making a valid update deposit request api call.");

let response = apis::deposit_api::get_deposit(
&configuration,
bitcoin_txid,
&bitcoin_tx_output_index.to_string(),
)
.await
.expect("Received an error after making a valid get deposit api call.");
assert_eq!(response.bitcoin_txid, bitcoin_txid);
assert_eq!(response.status, status);

if should_reject {
apis::deposit_api::create_deposit(&configuration, create_deposit_body)
.await
.expect_err(&format!(
"We should reject duplicate deposits, if old one is {:#?}",
status
));
} else {
apis::deposit_api::create_deposit(&configuration, create_deposit_body)
.await
.expect("Received an error after making a valid get deposit api call.");
}

let response = apis::deposit_api::get_deposit(
&configuration,
bitcoin_txid,
&bitcoin_tx_output_index.to_string(),
)
.await
.expect("Received an error after making a valid get deposit api call.");
assert_eq!(response.bitcoin_txid, bitcoin_txid);
let expected_status = if status == Status::Reprocessing {
Status::Pending
} else {
status
};
assert_eq!(response.status, expected_status);
}

// Maybe looks weird from the first glance, but I like that this won't compile if
// a new option to Status enum will be added without updating this test.
for status in [
Status::Accepted,
Status::Confirmed,
Status::Failed,
Status::Pending,
Status::Reprocessing,
] {
match status {
Status::Accepted | Status::Confirmed | Status::Failed => {
test_duplicate(status, true).await;
}
Status::Pending | Status::Reprocessing => {
test_duplicate(status, false).await;
}
}
}
matteojug marked this conversation as resolved.
Show resolved Hide resolved
}
Loading