Skip to content

Commit

Permalink
Add extension verify tests (#949)
Browse files Browse the repository at this point in the history
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

- **New Features**
- Introduced dynamic user registration upon their first deposit to
enhance user experience.
  
- **Refactor**
- Enhanced signature verification for withdrawal requests to support
both proxy and main accounts, improving security and error handling.

- **Tests**
- Added new tests for signature verification in orders and withdrawal
requests.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
  • Loading branch information
Gauthamastro authored Apr 29, 2024
2 parents d288202 + 846fdf8 commit b89e840
Show file tree
Hide file tree
Showing 3 changed files with 74 additions and 50 deletions.
8 changes: 6 additions & 2 deletions pallets/ocex/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1706,12 +1706,16 @@ pub mod pallet {
) -> DispatchResult {
ensure!(Self::orderbook_operational_state(), Error::<T>::ExchangeNotOperational);
ensure!(<AllowlistedToken<T>>::get().contains(&asset), Error::<T>::TokenNotAllowlisted);
// Check if account is registered
ensure!(<Accounts<T>>::contains_key(&user), Error::<T>::AccountNotRegistered);
ensure!(amount.saturated_into::<u128>() <= DEPOSIT_MAX, Error::<T>::AmountOverflow);
let converted_amount = Decimal::from(amount.saturated_into::<u128>())
.checked_div(Decimal::from(UNIT_BALANCE))
.ok_or(Error::<T>::FailedToConvertDecimaltoBalance)?;

// if a new user is depositing, then register the user with main account as proxy
if !<Accounts<T>>::contains_key(&user) {
Self::register_user(user.clone(), user.clone())?;
}

Self::transfer_asset(&user, &Self::get_pallet_account(), amount, asset)?;
// Get Storage Map Value
if let Some(expected_total_amount) =
Expand Down
16 changes: 7 additions & 9 deletions pallets/ocex/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1509,15 +1509,13 @@ fn test_deposit_account_not_registered() {
let account_id = create_account_id();
new_test_ext().execute_with(|| {
assert_ok!(OCEX::set_exchange_state(RuntimeOrigin::root(), true));
allowlist_token(AssetId::Asset(10));
assert_noop!(
OCEX::deposit(
RuntimeOrigin::signed(account_id.clone().into()),
AssetId::Asset(10),
100_u128.into()
),
Error::<Test>::AccountNotRegistered
);
mint_into_account(account_id.clone());
allowlist_token(AssetId::Polkadex);
assert_ok!(OCEX::deposit(
RuntimeOrigin::signed(account_id.clone().into()),
AssetId::Polkadex,
100_u128.into()
));
});
}

Expand Down
100 changes: 61 additions & 39 deletions primitives/orderbook/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -295,11 +295,32 @@ impl<AccountId: Clone + Codec + TypeInfo> WithdrawalRequest<AccountId> {
impl<AccountId: Codec + Clone + TypeInfo> WithdrawalRequest<AccountId> {
/// Verifies request payload.
pub fn verify(&self) -> bool {
let signer = match Decode::decode(&mut &self.proxy.encode()[..]) {
Ok(signer) => signer,
Err(_) => return false,
};
self.signature.verify(self.payload.encode().as_ref(), &signer)
// check signature with proxy account
let signer = Decode::decode(&mut &self.proxy.encode()[..]);
let mut result = false;
if let Ok(signer) = signer {
result = self.signature.verify(self.payload.encode().as_ref(), &signer);
}
if result {
return true;
}
log::error!(target:"orderbook","Withdrawal request signature check failed");

// check signature with main account
let signer = Decode::decode(&mut &self.main.encode()[..]);
match signer {
Ok(main) => {
let payload_str = serde_json::to_string(&self.payload);
if let Ok(payload_str) = payload_str {
return self.signature.verify_extension_signature(&payload_str, &main);
}
false
},
Err(err) => {
log::error!(target:"orderbook","Withdrawal request signature check failed {:}", err);
false
},
}
}

/// Instantiates `AccountAsset` DTO based on owning data.
Expand Down Expand Up @@ -982,39 +1003,6 @@ impl TryFrom<OrderDetails> for Order {
}
}

/// Defines withdraw details DTO.
#[derive(Clone, Debug, Encode, Decode, Eq, PartialEq, Serialize, Deserialize)]
pub struct WithdrawalDetails {
/// Withdraw payload.
pub payload: WithdrawPayloadCallByUser,
/// Main account identifier.
pub main: AccountId,
/// Proxy account identifier.
pub proxy: AccountId,
/// Signature.
pub signature: Signature,
}

impl WithdrawalDetails {
/// Verifies the signature.
pub fn verify_signature(&self) -> bool {
let result = self.signature.verify(self.payload.encode().as_ref(), &self.proxy);
if result {
return true;
}
log::error!(target:"orderbook","Withdrawal signature check failed");
let payload_str = serde_json::to_string(&self.payload);
if let Ok(payload_str) = payload_str {
let result = self.signature.verify_extension_signature(&payload_str, &self.main);
if result {
return true;
}
}
log::error!(target:"orderbook","Withdrawal extension signature check failed");
false
}
}

/// Overarching type used by validators when submitting
/// their signature for a summary to aggregator
#[derive(serde::Serialize, serde::Deserialize, Debug, Clone)]
Expand All @@ -1031,7 +1019,10 @@ pub struct ApprovedSnapshot {
mod tests {
use crate::ingress::{EgressMessages, IngressMessages};
use crate::traits::VerifyExtensionSignature;
use crate::types::UserActions;
use crate::types::{
Order, OrderDetails, OrderPayload, UserActions, WithdrawPayloadCallByUser,
WithdrawalRequest,
};
use polkadex_primitives::{AccountId, AssetId};
use rust_decimal::Decimal;
use sp_runtime::MultiSignature;
Expand Down Expand Up @@ -1068,4 +1059,35 @@ mod tests {
let result = sig.verify_extension_signature(&payload, &account);
assert_eq!(result, true);
}

#[test]
pub fn verify_order_signed_by_extension() {
let order_payload_str = "{\"client_order_id\":\"0x7765626170702d000079a87e313975c2490257e1ea808147fd0d7a096930b4c3\",\"user\":\"5Cct7e6gLzXHN35Zc9QYqA1DXPeJFhqt3RiZGzCMzo16JwjC\",\"main_account\":\"5FYr5g1maSsAAw6w98xdAytZ6MEQ8sNPgp3PNLgy9o79kMug\",\"pair\":\"PDEX-3496813586714279103986568049643838918\",\"side\":\"Ask\",\"order_type\":\"LIMIT\",\"quote_order_quantity\":\"0\",\"qty\":\"1\",\"price\":\"1\",\"timestamp\":1714158182636}";
let signature_payload_str = "{\"Sr25519\":\"32ce7e9d9ca9eb84447a079e5309e313a3a6767211c5b5957d6512825f0d2f00dcccc1ca57cc514e9a82d605431e989b03bbceca29a421e515023f138ea6ff84\"}";
let payload = serde_json::from_str::<OrderPayload>(order_payload_str).unwrap();
let signature = serde_json::from_str::<MultiSignature>(signature_payload_str).unwrap();
// Convert to Order type for primitives
let order_details = OrderDetails { payload: payload.clone(), signature: signature.clone() };
let order: Order = Order::try_from(order_details).unwrap();
assert_eq!(order.verify_signature(), true);
}

#[test]
pub fn verify_withdrawal_request_signed_by_extension() {
let withdraw_payload_str =
"{\"asset_id\":{\"asset\":\"PDEX\"},\"amount\":\"1.11111111\",\"timestamp\":1714229288928}";
let signature_payload_str =
"{\"Sr25519\":\"785ae7c0ece6fb07429689f0b7d30f11e8f612507fbbc4edb3cbc668f7b4d3060a460b32ae2d4fed52b97faf21d9de768881d25711c9141fde40af4d58e57886\"}";
let payload =
serde_json::from_str::<WithdrawPayloadCallByUser>(withdraw_payload_str).unwrap();
let signature = serde_json::from_str::<MultiSignature>(signature_payload_str).unwrap();
const MAIN_ACCOUNT: &str = "5FYr5g1maSsAAw6w98xdAytZ6MEQ8sNPgp3PNLgy9o79kMug";
let request = WithdrawalRequest {
payload: payload.clone(),
main: AccountId::from_str(MAIN_ACCOUNT).unwrap(),
proxy: AccountId::from_str(MAIN_ACCOUNT).unwrap(),
signature: signature.clone(),
};
assert_eq!(request.verify(), true);
}
}

0 comments on commit b89e840

Please sign in to comment.