From 331383ed132caad6d96ec884b7a32283abf23d7c Mon Sep 17 00:00:00 2001 From: Hamish Peebles Date: Fri, 15 Sep 2023 21:58:41 +0100 Subject: [PATCH 1/3] Use user specific subaccount when transferring to bot users --- .../user/impl/src/updates/send_message.rs | 30 +++++++++++-------- backend/libraries/types/src/cryptocurrency.rs | 22 +++++++++----- 2 files changed, 32 insertions(+), 20 deletions(-) diff --git a/backend/canisters/user/impl/src/updates/send_message.rs b/backend/canisters/user/impl/src/updates/send_message.rs index 42e63d4196..9b4137a649 100644 --- a/backend/canisters/user/impl/src/updates/send_message.rs +++ b/backend/canisters/user/impl/src/updates/send_message.rs @@ -2,6 +2,7 @@ use crate::crypto::process_transaction_without_caller_check; use crate::guards::caller_is_owner; use crate::timer_job_types::RetrySendingFailedMessagesJob; use crate::{mutate_state, read_state, run_regular_jobs, RuntimeState, TimerJob}; +use candid::Principal; use canister_tracing_macros::trace; use chat_events::{PushMessageArgs, Reader}; use ic_cdk_macros::update; @@ -24,16 +25,18 @@ use utils::time::{MINUTE_IN_MS, SECOND_IN_MS}; async fn send_message_v2(mut args: Args) -> Response { run_regular_jobs(); - let user_type = match read_state(|state| validate_request(&args, state)) { - ValidateRequestResult::Valid(t) => t, + let (my_user_id, user_type) = match read_state(|state| validate_request(&args, state)) { + ValidateRequestResult::Valid(u, t) => (u, t), ValidateRequestResult::Invalid(response) => return response, - ValidateRequestResult::RecipientUnknown(local_user_index_canister_id) => { + ValidateRequestResult::RecipientUnknown(u, local_user_index_canister_id) => { let c2c_args = local_user_index_canister::c2c_lookup_user::Args { user_id_or_principal: args.recipient.into(), }; match local_user_index_canister_c2c_client::c2c_lookup_user(local_user_index_canister_id, &c2c_args).await { - Ok(local_user_index_canister::c2c_lookup_user::Response::Success(result)) if result.is_bot => UserType::Bot, - Ok(local_user_index_canister::c2c_lookup_user::Response::Success(_)) => UserType::User, + Ok(local_user_index_canister::c2c_lookup_user::Response::Success(result)) if result.is_bot => { + (u, UserType::Bot) + } + Ok(local_user_index_canister::c2c_lookup_user::Response::Success(_)) => (u, UserType::User), Ok(local_user_index_canister::c2c_lookup_user::Response::UserNotFound) => return RecipientNotFound, Err(error) => return InternalError(format!("{error:?}")), } @@ -47,13 +50,16 @@ async fn send_message_v2(mut args: Args) -> Response { if user_type.is_self() { return InvalidRequest("Cannot send crypto to yourself".to_string()); } - let pending_transaction = match &c.transfer { + let mut pending_transaction = match &c.transfer { CryptoTransaction::Pending(t) => t.clone(), _ => return InvalidRequest("Transaction must be of type 'Pending'".to_string()), }; - if !pending_transaction.is_user_recipient(args.recipient) { + if !pending_transaction.validate_recipient(args.recipient) { return InvalidRequest("Transaction is not to the user's account".to_string()); } + if user_type.is_bot() { + pending_transaction.set_recipient(args.recipient.into(), Principal::from(my_user_id).into()); + } // We have to use `process_transaction_without_caller_check` because we may be within a // reply callback due to calling `c2c_lookup_user` earlier. @@ -87,9 +93,9 @@ impl UserType { #[allow(clippy::large_enum_variant)] enum ValidateRequestResult { - Valid(UserType), + Valid(UserId, UserType), Invalid(Response), - RecipientUnknown(CanisterId), // Value is the user_index canisterId + RecipientUnknown(UserId, CanisterId), // UserId, UserIndexCanisterId } fn validate_request(args: &Args, state: &RuntimeState) -> ValidateRequestResult { @@ -130,12 +136,12 @@ fn validate_request(args: &Args, state: &RuntimeState) -> ValidateRequestResult } }) } else if args.recipient == my_user_id { - ValidateRequestResult::Valid(UserType::_Self) + ValidateRequestResult::Valid(my_user_id, UserType::_Self) } else if let Some(chat) = state.data.direct_chats.get(&args.recipient.into()) { let user_type = if chat.is_bot { UserType::Bot } else { UserType::User }; - ValidateRequestResult::Valid(user_type) + ValidateRequestResult::Valid(my_user_id, user_type) } else { - ValidateRequestResult::RecipientUnknown(state.data.local_user_index_canister_id) + ValidateRequestResult::RecipientUnknown(my_user_id, state.data.local_user_index_canister_id) } } diff --git a/backend/libraries/types/src/cryptocurrency.rs b/backend/libraries/types/src/cryptocurrency.rs index 097da0063a..7a38a980bd 100644 --- a/backend/libraries/types/src/cryptocurrency.rs +++ b/backend/libraries/types/src/cryptocurrency.rs @@ -1,6 +1,7 @@ +use crate::nns::UserOrAccount; use crate::{CanisterId, TimestampNanos, UserId}; use candid::{CandidType, Principal}; -use ic_ledger_types::Subaccount; +use ic_ledger_types::{AccountIdentifier, Subaccount, DEFAULT_SUBACCOUNT}; use serde::{Deserialize, Serialize}; #[derive(CandidType, Serialize, Deserialize, Clone, Debug, Eq, PartialEq, Hash)] @@ -60,7 +61,6 @@ impl Cryptocurrency { } pub type TransactionHash = [u8; 32]; -pub const DEFAULT_SUBACCOUNT: &[u8; 32] = &[0; 32]; #[derive(CandidType, Serialize, Deserialize, Clone, Debug)] pub enum CryptoTransaction { @@ -143,16 +143,22 @@ impl PendingCryptoTransaction { } } - pub fn is_user_recipient(&self, user_id: UserId) -> bool { + pub fn validate_recipient(&self, recipient: UserId) -> bool { match self { PendingCryptoTransaction::NNS(t) => match t.to { - nns::UserOrAccount::User(u) => u == user_id, - nns::UserOrAccount::Account(a) => { - a == ic_ledger_types::AccountIdentifier::new(&user_id.into(), &Subaccount(*DEFAULT_SUBACCOUNT)) - } + UserOrAccount::Account(a) => a == AccountIdentifier::new(&recipient.into(), &DEFAULT_SUBACCOUNT), + UserOrAccount::User(u) => u == recipient, }, + PendingCryptoTransaction::ICRC1(t) => t.to.owner == recipient.into(), + } + } + + pub fn set_recipient(&mut self, owner: Principal, subaccount: Subaccount) { + match self { + PendingCryptoTransaction::NNS(t) => t.to = UserOrAccount::Account(AccountIdentifier::new(&owner, &subaccount)), PendingCryptoTransaction::ICRC1(t) => { - t.to.owner == user_id.into() && t.to.subaccount.map_or(true, |s| s == *DEFAULT_SUBACCOUNT) + t.to.owner = owner; + t.to.subaccount = Some(subaccount.0) } } } From 0d47816b808acd24a1f5df16bcef62c0b58a2fac Mon Sep 17 00:00:00 2001 From: Hamish Peebles Date: Fri, 15 Sep 2023 22:00:37 +0100 Subject: [PATCH 2/3] Update CHANGELOG --- backend/canisters/user/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/backend/canisters/user/CHANGELOG.md b/backend/canisters/user/CHANGELOG.md index 0123f47de5..251430708f 100644 --- a/backend/canisters/user/CHANGELOG.md +++ b/backend/canisters/user/CHANGELOG.md @@ -14,6 +14,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - Include username and display name updates in updates loop ([#4343](https://github.com/open-chat-labs/open-chat/pull/4343)) - Add `last_updated` to direct chat summaries ([#4364](https://github.com/open-chat-labs/open-chat/pull/4364)) - Add `rules_accepted` to cached group summaries ([#4366](https://github.com/open-chat-labs/open-chat/pull/4366)) +- Transfer to user specific subaccounts when transferring to bot users ([#4388](https://github.com/open-chat-labs/open-chat/pull/4388)) ## [[2.0.838](https://github.com/open-chat-labs/open-chat/releases/tag/v2.0.838-user)] - 2023-09-05 From 299d2967b7e2d0a744a98ff0f7ee05989631784a Mon Sep 17 00:00:00 2001 From: Hamish Peebles Date: Fri, 15 Sep 2023 22:02:23 +0100 Subject: [PATCH 3/3] Add comment --- backend/canisters/user/impl/src/updates/send_message.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/backend/canisters/user/impl/src/updates/send_message.rs b/backend/canisters/user/impl/src/updates/send_message.rs index 9b4137a649..a66df707ab 100644 --- a/backend/canisters/user/impl/src/updates/send_message.rs +++ b/backend/canisters/user/impl/src/updates/send_message.rs @@ -57,6 +57,8 @@ async fn send_message_v2(mut args: Args) -> Response { if !pending_transaction.validate_recipient(args.recipient) { return InvalidRequest("Transaction is not to the user's account".to_string()); } + // When transferring to bot users, each user transfers to their own subaccount, this way it + // is trivial for the bots to keep track of each user's funds if user_type.is_bot() { pending_transaction.set_recipient(args.recipient.into(), Principal::from(my_user_id).into()); }