Skip to content

Commit

Permalink
Atomic account creation (#257)
Browse files Browse the repository at this point in the history 
* atomic account creation tests

* register_account_atomic (broken)

* Rebase off of latest relayer

* NUM_KEYS set to 1

* atomic url fixed

* return type in atomic acc creation fixed

* atomic acc creation status error fxed
---------

Co-authored-by: Phuong Nguyen <[email protected]>
  • Loading branch information
@volovyks @ChaoticTempest
volovyks and ChaoticTempest authored Sep 19, 2023
1 parent f2fe300 commit 1b0e3d3
Show file tree
Hide file tree
Showing 5 changed files with 238 additions and 60 deletions.
49 changes: 49 additions & 0 deletions integration-tests/tests/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,8 @@ trait MpcCheck {
fn assert_ok(self) -> anyhow::Result<Self::Response>;
fn assert_bad_request_contains(self, expected: &str) -> anyhow::Result<Self::Response>;
fn assert_unauthorized_contains(self, expected: &str) -> anyhow::Result<Self::Response>;
fn assert_internal_error_contains(self, expected: &str) -> anyhow::Result<Self::Response>;
fn assert_dependency_error_contains(self, expected: &str) -> anyhow::Result<Self::Response>;

fn assert_bad_request(self) -> anyhow::Result<Self::Response>
where
Expand All @@ -230,6 +232,12 @@ trait MpcCheck {
{
self.assert_unauthorized_contains("")
}
fn assert_internal_error(self) -> anyhow::Result<Self::Response>
where
Self: Sized,
{
self.assert_internal_error_contains("")
}
}

// Presumes that $response::Err has a `msg: String` field.
Expand Down Expand Up @@ -297,6 +305,47 @@ macro_rules! impl_mpc_check {
);
}
}
// ideally we should not have situations where we can get INTERNAL_SERVER_ERROR
fn assert_internal_error_contains(
self,
expected: &str,
) -> anyhow::Result<Self::Response> {
let status_code = self.0;
let response = self.1;

if status_code == StatusCode::INTERNAL_SERVER_ERROR {
let $response::Err { ref msg, .. } = response else {
anyhow::bail!("unexpected error with a 401 http code");
};
assert!(msg.contains(expected));

Ok(response)
} else {
anyhow::bail!(
"expected 401, but got {status_code} with response: {response:?}"
);
}
}
fn assert_dependency_error_contains(
self,
expected: &str,
) -> anyhow::Result<Self::Response> {
let status_code = self.0;
let response = self.1;

if status_code == StatusCode::FAILED_DEPENDENCY {
let $response::Err { ref msg, .. } = response else {
anyhow::bail!("unexpected error with a 424 http code");
};
assert!(msg.contains(expected));

Ok(response)
} else {
anyhow::bail!(
"expected 424, but got {status_code} with response: {response:?}"
);
}
}
}
};
}
Expand Down
104 changes: 92 additions & 12 deletions integration-tests/tests/mpc/negative.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -206,18 +206,16 @@ async fn negative_front_running_protection() -> anyhow::Result<()> {
let wrong_oidc_token = OidcToken::random();

// Create account before claiming OIDC token
// This part of the test is commented since account creation is not atomic (known issue)
// Relayer is wasting a token even if account was not created
// ctx.leader_node
// .new_account_with_helper(
// account_id.to_string(),
// user_public_key.clone(),
// None,
// user_secret_key.clone(),
// oidc_token_1.clone(),
// )
// .await?
// .assert_unauthorized_contains("was not claimed")?;
ctx.leader_node
.new_account_with_helper(
&account_id,
&user_public_key,
None,
&user_secret_key,
&oidc_token_1,
)
.await?
.assert_unauthorized_contains("was not claimed")?;

// Get user recovery PK before claiming OIDC token
ctx.leader_node
Expand Down Expand Up @@ -605,3 +603,85 @@ async fn test_malformed_raw_create_account() -> anyhow::Result<()> {
})
.await
}

#[test(tokio::test)]
async fn test_account_creation_should_work_on_second_attempt() -> anyhow::Result<()> {
with_nodes(2, |ctx| {
Box::pin(async move {
let account_id = account::random(ctx.worker)?;
let user_secret_key = key::random_sk();
let user_public_key = user_secret_key.public_key();
let oidc_token = OidcToken::random();

ctx.leader_node
.new_account_with_helper(
&account_id,
&user_public_key,
None,
&user_secret_key,
&oidc_token,
)
.await?
.assert_unauthorized_contains("was not claimed")?;

register_account(
&ctx,
&account_id,
&user_secret_key,
&user_public_key,
&oidc_token,
None,
)
.await?;

Ok(())
})
})
.await
}

#[test(tokio::test)]
async fn test_creation_of_two_account_with_the_same_oidc_should_not_be_possible(
) -> anyhow::Result<()> {
with_nodes(2, |ctx| {
Box::pin(async move {
let account_id = account::random(ctx.worker)?;
let account_id_2 = account::random(ctx.worker)?;
let user_secret_key = key::random_sk();
let user_public_key = user_secret_key.public_key();
let oidc_token = OidcToken::random();

ctx.leader_node
.claim_oidc_with_helper(&oidc_token, &user_public_key, &user_secret_key)
.await?
.assert_ok()?;

ctx.leader_node
.new_account_with_helper(
&account_id,
&user_public_key,
None,
&user_secret_key,
&oidc_token,
)
.await?
.assert_ok()?;

ctx.leader_node
.new_account_with_helper(
&account_id_2,
&user_public_key,
None,
&user_secret_key,
&oidc_token,
)
.await?
.assert_dependency_error_contains(
"You can only register 1 account per oauth_token",
)?;

Ok(())
})
})
.await
}
85 changes: 39 additions & 46 deletions mpc-recovery/src/leader_node/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use crate::msg::{
SignRequest, SignResponse, UserCredentialsRequest, UserCredentialsResponse,
};
use crate::oauth::OAuthTokenVerifier;
use crate::relayer::msg::RegisterAccountRequest;
use crate::relayer::msg::{CreateAccountAtomicRequest, RegisterAccountRequest};
use crate::relayer::NearRpcAndRelayerClient;
use crate::transaction::{
get_create_account_delegate_action, get_local_signed_delegated_action, get_mpc_signature,
Expand All @@ -32,7 +32,6 @@ use near_crypto::SecretKey;
use near_primitives::delegate_action::{DelegateAction, NonDelegateAction};
use near_primitives::transaction::{Action, DeleteKeyAction};
use near_primitives::types::AccountId;
use near_primitives::views::FinalExecutionStatus;
use prometheus::{Encoder, TextEncoder};
use rand::{distributions::Alphanumeric, Rng};
use std::net::SocketAddr;
Expand Down Expand Up @@ -356,22 +355,6 @@ async fn process_new_account<T: OAuthTokenVerifier>(
.map_err(LeaderNodeError::SignatureVerificationFailed)?;
tracing::debug!("user credentials digest signature verified for {new_user_account_id:?}");

let partner = state
.partners
.find(&oidc_token_claims.iss, &oidc_token_claims.aud)?;

state
.client
.register_account(
RegisterAccountRequest {
account_id: new_user_account_id.clone(),
allowance: 300_000_000_000_000,
oauth_token: internal_acc_id.clone(),
},
partner.relayer,
)
.await?;

nar::retry(|| async {
// Get nonce and recent block hash
let (_hash, block_height, nonce) = state
Expand Down Expand Up @@ -417,40 +400,50 @@ async fn process_new_account<T: OAuthTokenVerifier>(
);

// Send delegate action to relayer
let request = CreateAccountAtomicRequest {
account_id: new_user_account_id.clone(),
allowance: 300_000_000_000_000,
oauth_token: internal_acc_id.clone(),
signed_delegate_action,
};

// TODO: move error message from here to this place
let partner = state
.partners
.find(&oidc_token_claims.iss, &oidc_token_claims.aud)?;

let result = state
.client
.send_meta_tx(signed_delegate_action, partner.relayer)
.create_account_atomic(request, partner.relayer)
.await;
if let Err(err) = &result {
let err_str = format!("{:?}", err);
state
.client
.invalidate_cache_if_tx_failed(
&(
state.account_creator_id.clone(),
state.account_creator_sk.public_key(),
),
&err_str,
)
.await;
}
let response = result?;

// TODO: Probably need to check more fields
if matches!(response.status, FinalExecutionStatus::SuccessValue(_)) {
Ok(NewAccountResponse::Ok {
create_account_options: new_account_options,
user_recovery_public_key: mpc_user_recovery_pk,
near_account_id: new_user_account_id.clone(),
})
} else {
Err(LeaderNodeError::Other(anyhow::anyhow!(
"transaction failed with {:?}",
response.status
)))

match result {
Ok(_) => {
tracing::info!(
"account creation succeeded: {new_user_account_id:?}",
new_user_account_id = new_user_account_id
);
Ok(NewAccountResponse::Ok {
create_account_options: new_account_options,
user_recovery_public_key: mpc_user_recovery_pk,
near_account_id: new_user_account_id.clone(),
})
}
Err(err) => {
tracing::error!("account creation failed: {err}");
let err_str = format!("{:?}", err);
state
.client
.invalidate_cache_if_acc_creation_failed(
&(
state.account_creator_id.clone(),
state.account_creator_sk.public_key(),
),
&err_str,
)
.await;
Err(LeaderNodeError::RelayerError(err))
}
}
})
.await
Expand Down
50 changes: 48 additions & 2 deletions mpc-recovery/src/relayer/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ use near_primitives::types::{AccountId, BlockHeight, Nonce};
use near_primitives::views::FinalExecutionStatus;

use self::error::RelayerError;
use self::msg::{RegisterAccountRequest, SendMetaTxRequest, SendMetaTxResponse};
use self::msg::{
CreateAccountAtomicRequest, RegisterAccountRequest, SendMetaTxRequest, SendMetaTxResponse,
};

use crate::firewall::allowed::DelegateActionRelayer;
use crate::nar::{self, CachedAccessKeyNonces};
Expand Down Expand Up @@ -95,6 +97,50 @@ impl NearRpcAndRelayerClient {
}
}

#[tracing::instrument(level = "debug", skip_all, fields(account_id = request.account_id.to_string()))]
pub async fn create_account_atomic(
&self,
request: CreateAccountAtomicRequest,
relayer: DelegateActionRelayer,
) -> Result<(), RelayerError> {
let mut req = Request::builder()
.method(Method::POST)
.uri(format!("{}/create_account_atomic", relayer.url))
.header("content-type", "application/json");

if let Some(api_key) = relayer.api_key {
req = req.header("x-api-key", api_key);
};

let request = req
.body(Body::from(
serde_json::to_vec(&request)
.map_err(|e| RelayerError::DataConversionFailure(e.into()))?,
))
.map_err(|e| RelayerError::NetworkFailure(e.into()))?;

tracing::debug!("constructed http request to {}", relayer.url);
let client = Client::new();
let response = client
.request(request)
.await
.map_err(|e| RelayerError::NetworkFailure(e.into()))?;

let status = response.status();
let response_body = hyper::body::to_bytes(response.into_body())
.await
.map_err(|e| RelayerError::NetworkFailure(e.into()))?;
let msg = std::str::from_utf8(&response_body)
.map_err(|e| RelayerError::DataConversionFailure(e.into()))?;

if status.is_success() {
tracing::debug!(response_body = msg, "got response");
Ok(())
} else {
Err(RelayerError::RequestFailure(status, msg.to_string()))
}
}

#[tracing::instrument(level = "debug", skip_all, fields(receiver_id = request.delegate_action.receiver_id.to_string()))]
pub async fn send_meta_tx(
&self,
Expand Down Expand Up @@ -149,7 +195,7 @@ impl NearRpcAndRelayerClient {
}
}

pub(crate) async fn invalidate_cache_if_tx_failed(
pub(crate) async fn invalidate_cache_if_acc_creation_failed(
&self,
cache_key: &(AccountId, PublicKey),
err_str: &str,
Expand Down
10 changes: 10 additions & 0 deletions mpc-recovery/src/relayer/msg.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,16 @@ pub struct RegisterAccountRequest {
pub oauth_token: String,
}

#[derive(Clone, Debug, Serialize, Deserialize)]
pub struct CreateAccountAtomicRequest {
pub account_id: AccountId,
pub allowance: u64,
// This is actually an InternalAccountId.
// TODO: rename it to internal_account_id on the relayer side
pub oauth_token: String,
pub signed_delegate_action: SignedDelegateAction,
}

pub type SendMetaTxRequest = SignedDelegateAction;

#[derive(Clone, Debug, Serialize, Deserialize)]
Expand Down

0 comments on commit 1b0e3d3

Please sign in to comment.