From 27facc713f260938669da5bc2639db612e94eb82 Mon Sep 17 00:00:00 2001 From: olaszakos Date: Thu, 29 Feb 2024 12:08:51 +0100 Subject: [PATCH] feat: add transaction hash to executed ICP transfer (#150) Bundled in this PR: * implement producing transaction hash for ICP ledger based on [rosetta_api implementation](https://github.com/dfinity/ic/blob/e1ba803a9219257bc1be0892e2a72c1fd3831f50/rs/rosetta-api/icp_ledger/src/lib.rs#L234) * extend default access policy to allow account owners to read transfers, otherwise list_account_transfers will fail for the account owners * add transfer_id to TransferOperation * add tests to check if the transaction_hash is filled out, and that executed transfer proposals have the transfer id --- Cargo.lock | 17 ++++ Cargo.toml | 1 + .../integration-tests/src/transfer_tests.rs | 44 ++++++++++- .../ui/src/generated/wallet/wallet.did.d.ts | 1 + .../ui/src/generated/wallet/wallet.did.js | 1 + canisters/wallet/api/spec.did | 2 + canisters/wallet/api/src/transfer.rs | 1 + canisters/wallet/impl/Cargo.toml | 1 + .../wallet/impl/src/core/access_control.rs | 5 +- .../impl/src/factories/blockchains/core.rs | 3 + .../blockchains/internet_computer.rs | 77 ++++++++++++++++--- .../src/jobs/execute_created_transfers.rs | 42 +++++++--- .../impl/src/mappers/proposal_operation.rs | 3 + 13 files changed, 171 insertions(+), 27 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2892b93b0..09114a53c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -663,6 +663,12 @@ dependencies = [ "tracing", ] +[[package]] +name = "half" +version = "1.8.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1b43ede17f21864e81be2fa654110bf1e793774238d86ef8555c37e6519c0403" + [[package]] name = "hashbrown" version = "0.14.3" @@ -1707,6 +1713,16 @@ dependencies = [ "serde", ] +[[package]] +name = "serde_cbor" +version = "0.11.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2bef2ebfde456fb76bbcf9f59315333decc4fda0b2b44b420243c11e0f5ec1f5" +dependencies = [ + "half", + "serde", +] + [[package]] name = "serde_derive" version = "1.0.193" @@ -2331,6 +2347,7 @@ dependencies = [ "rstest", "serde", "serde_bytes", + "serde_cbor", "sha2", "thiserror", "tokio", diff --git a/Cargo.toml b/Cargo.toml index 55f94e7c6..e557552af 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -50,6 +50,7 @@ rstest = "0.18.2" serde = "1.0.188" serde_bytes = "0.11" serde_json = "1.0" +serde_cbor = "0.11.2" sha2 = "0.10" syn = { version = "2.0", features = ["extra-traits", "full"] } thiserror = "1.0.48" diff --git a/canisters/integration-tests/src/transfer_tests.rs b/canisters/integration-tests/src/transfer_tests.rs index bb6dcd3dc..ef5ca61cb 100644 --- a/canisters/integration-tests/src/transfer_tests.rs +++ b/canisters/integration-tests/src/transfer_tests.rs @@ -6,13 +6,14 @@ use crate::utils::user_test_id; use crate::TestEnv; use ic_canister_core::api::ApiResult; use ic_ledger_types::AccountIdentifier; -use pocket_ic::update_candid_as; +use pocket_ic::{query_candid_as, update_candid_as}; use std::time::Duration; use wallet_api::{ AccountPoliciesDTO, AddAccountOperationInput, ApiErrorDTO, ApprovalThresholdDTO, CreateProposalInput, CreateProposalResponse, CriteriaDTO, GetProposalInput, - GetProposalResponse, MeResponse, ProposalExecutionScheduleDTO, ProposalOperationDTO, - ProposalOperationInput, ProposalStatusDTO, TransferOperationInput, UserSpecifierDTO, + GetProposalResponse, ListAccountTransfersInput, ListAccountTransfersResponse, MeResponse, + ProposalExecutionScheduleDTO, ProposalOperationDTO, ProposalOperationInput, ProposalStatusDTO, + TransferOperationInput, UserSpecifierDTO, }; #[test] @@ -129,7 +130,7 @@ fn make_transfer_successful() { // make transfer proposal to beneficiary let transfer = TransferOperationInput { - from_account_id: account_dto.id, + from_account_id: account_dto.id.clone(), to: default_account(beneficiary_id), amount: ICP.into(), fee: None, @@ -183,7 +184,42 @@ fn make_transfer_successful() { } }; + // proposal has the transfer id filled out + match new_proposal_dto.operation { + ProposalOperationDTO::Transfer(transfer) => { + transfer.transfer_id.expect("transfer id must be set") + } + _ => { + panic!("proposal must be Transfer"); + } + }; + // check beneficiary balance after completed transfer let new_beneficiary_balance = get_icp_balance(&env, beneficiary_id); assert_eq!(new_beneficiary_balance, ICP); + + // load account transfers + let res: (Result,) = query_candid_as( + &env, + canister_ids.wallet, + WALLET_ADMIN_USER, + "list_account_transfers", + (ListAccountTransfersInput { + account_id: account_dto.id, + from_dt: None, + to_dt: None, + status: None, + },), + ) + .unwrap(); + + // transactions should be completed and have transaction hash + let all_have_transaction_hash = res.0.unwrap().transfers.iter().all(|t| match &t.status { + wallet_api::TransferStatusDTO::Completed { hash, .. } => hash.is_some(), + _ => { + panic!("transfer should be completed"); + } + }); + + assert!(all_have_transaction_hash); } diff --git a/canisters/ui/src/generated/wallet/wallet.did.d.ts b/canisters/ui/src/generated/wallet/wallet.did.d.ts index 560d2e31c..e589f1602 100644 --- a/canisters/ui/src/generated/wallet/wallet.did.d.ts +++ b/canisters/ui/src/generated/wallet/wallet.did.d.ts @@ -671,6 +671,7 @@ export interface TransferListItem { export interface TransferMetadata { 'key' : string, 'value' : string } export interface TransferOperation { 'network' : Network, + 'transfer_id' : [] | [UUID], 'from_account' : [] | [Account], 'input' : TransferOperationInput, } diff --git a/canisters/ui/src/generated/wallet/wallet.did.js b/canisters/ui/src/generated/wallet/wallet.did.js index e507a96bd..b71322ac9 100644 --- a/canisters/ui/src/generated/wallet/wallet.did.js +++ b/canisters/ui/src/generated/wallet/wallet.did.js @@ -347,6 +347,7 @@ export const idlFactory = ({ IDL }) => { }); const TransferOperation = IDL.Record({ 'network' : Network, + 'transfer_id' : IDL.Opt(UUID), 'from_account' : IDL.Opt(Account), 'input' : TransferOperationInput, }); diff --git a/canisters/wallet/api/spec.did b/canisters/wallet/api/spec.did index d664cfd52..e8b443fe3 100644 --- a/canisters/wallet/api/spec.did +++ b/canisters/wallet/api/spec.did @@ -255,6 +255,8 @@ type TransferOperation = record { network : Network; // The input to the proposal to transfer funds. input : TransferOperationInput; + // The id of the executed transfer. + transfer_id : opt UUID; }; // Input type for editing an account through a proposal. diff --git a/canisters/wallet/api/src/transfer.rs b/canisters/wallet/api/src/transfer.rs index 234a0c522..d905c5e85 100644 --- a/canisters/wallet/api/src/transfer.rs +++ b/canisters/wallet/api/src/transfer.rs @@ -25,6 +25,7 @@ pub struct TransferOperationDTO { pub from_account: Option, pub network: NetworkDTO, pub input: TransferOperationInput, + pub transfer_id: Option, } #[derive(CandidType, Deserialize, Debug, Clone)] diff --git a/canisters/wallet/impl/Cargo.toml b/canisters/wallet/impl/Cargo.toml index f2a52961a..c1fe28e16 100644 --- a/canisters/wallet/impl/Cargo.toml +++ b/canisters/wallet/impl/Cargo.toml @@ -34,6 +34,7 @@ num-traits = { workspace = true } prometheus = { workspace = true } serde = { workspace = true } serde_bytes = { workspace = true } +serde_cbor = { workspace = true } sha2 = { workspace = true } thiserror = { workspace = true } uuid = { workspace = true, features = ["serde", "v4"] } diff --git a/canisters/wallet/impl/src/core/access_control.rs b/canisters/wallet/impl/src/core/access_control.rs index b298807df..2f0ff714c 100644 --- a/canisters/wallet/impl/src/core/access_control.rs +++ b/canisters/wallet/impl/src/core/access_control.rs @@ -438,7 +438,10 @@ impl Match<(User, ResourceSpecifier)> for AccessControlDefaultAccessMatcher { .iter() .all(|account| account.owners.contains(&caller.id)) } - ResourceSpecifier::Transfer(TransferActionSpecifier::Create(CommonSpecifier::Id( + ResourceSpecifier::Transfer(TransferActionSpecifier::Read(CommonSpecifier::Id( + account_ids, + ))) + | ResourceSpecifier::Transfer(TransferActionSpecifier::Create(CommonSpecifier::Id( account_ids, ))) => { let accounts = account_ids diff --git a/canisters/wallet/impl/src/factories/blockchains/core.rs b/canisters/wallet/impl/src/factories/blockchains/core.rs index 419efe152..4eb276bb9 100644 --- a/canisters/wallet/impl/src/factories/blockchains/core.rs +++ b/canisters/wallet/impl/src/factories/blockchains/core.rs @@ -8,6 +8,9 @@ use ic_canister_core::api::ApiError; use num_bigint::BigUint; use std::collections::HashMap; +pub const TRANSACTION_SUBMITTED_DETAILS_BLOCK_HEIGHT_KEY: &str = "block_height"; +pub const TRANSACTION_SUBMITTED_DETAILS_TRANSACTION_HASH_KEY: &str = "transaction_hash"; + pub type BlockchainApiResult = Result; #[derive(Clone, Debug, Hash)] diff --git a/canisters/wallet/impl/src/factories/blockchains/internet_computer.rs b/canisters/wallet/impl/src/factories/blockchains/internet_computer.rs index d1cc1a3ed..7e2141a11 100644 --- a/canisters/wallet/impl/src/factories/blockchains/internet_computer.rs +++ b/canisters/wallet/impl/src/factories/blockchains/internet_computer.rs @@ -1,5 +1,7 @@ use super::{ BlockchainApi, BlockchainApiResult, BlockchainTransactionFee, BlockchainTransactionSubmitted, + TRANSACTION_SUBMITTED_DETAILS_BLOCK_HEIGHT_KEY, + TRANSACTION_SUBMITTED_DETAILS_TRANSACTION_HASH_KEY, }; use crate::{ core::ic_cdk::api::id as wallet_canister_self_id, @@ -14,21 +16,21 @@ use byteorder::{BigEndian, ByteOrder}; use candid::Principal; use ic_canister_core::{ api::ApiError, - cdk::{self}, + cdk::{self, api::print}, }; use ic_ledger_types::{ - account_balance, transfer, AccountBalanceArgs, AccountIdentifier, Memo, Subaccount, Timestamp, - Tokens, TransferArgs, TransferError as LedgerTransferError, DEFAULT_FEE, + account_balance, query_blocks, transfer, AccountBalanceArgs, AccountIdentifier, GetBlocksArgs, + Memo, QueryBlocksResponse, Subaccount, Timestamp, Tokens, Transaction, TransferArgs, + TransferError as LedgerTransferError, DEFAULT_FEE, }; use num_bigint::BigUint; +use sha2::{Digest, Sha256}; use std::{ fmt::{Display, Formatter}, str::FromStr, }; use uuid::Uuid; -pub const ICP_TRANSACTION_SUBMITTED_DETAILS_BLOCK_HEIGHT_KEY: &str = "block_height"; - #[derive(Debug)] pub struct InternetComputer { /// This canister id is used to derive all the different wallet_accounts subaccount ids. @@ -58,6 +60,11 @@ impl Display for InternetComputerNetwork { } } +pub struct SubmitTransferResponse { + pub block_height: u64, + pub transaction_hash: Option, +} + impl InternetComputer { pub const BLOCKCHAIN: Blockchain = Blockchain::InternetComputer; pub const STANDARD: BlockchainStandard = BlockchainStandard::Native; @@ -75,6 +82,12 @@ impl InternetComputer { Principal::from_text(Self::ICP_LEDGER_CANISTER_ID).unwrap() } + fn hash_transaction(transaction: &Transaction) -> String { + let mut hasher = Sha256::new(); + hasher.update(&serde_cbor::ser::to_vec_packed(transaction).unwrap()); + hex::encode(hasher.finalize()) + } + /// Generates the corresponded subaccount id for the given wallet_account id. /// /// The subaccount id is a 32 bytes array that is used to identify a wallet_account in the ICP ledger. @@ -136,7 +149,7 @@ impl InternetComputer { &self, wallet_account: Account, wallet_transfer: Transfer, - ) -> Result { + ) -> Result { let current_time = cdk::api::time(); let amount: u64 = HelperMapper::biguint_to_u64(&wallet_transfer.amount.0)?; let transaction_fee: u64 = HelperMapper::biguint_to_u64(&wallet_transfer.fee.0)?; @@ -184,7 +197,41 @@ impl InternetComputer { }, })?; - Ok(block_height) + let transaction_hash = match query_blocks( + Self::ledger_canister_id(), + GetBlocksArgs { + length: 1, + start: block_height, + }, + ) + .await + { + Ok(QueryBlocksResponse { blocks, .. }) => { + let maybe_transaction_hash = blocks + .first() + .map(|block| Self::hash_transaction(&block.transaction)); + + print(format!( + "Error: no ICP ledger block found at height {}", + block_height + )); + + maybe_transaction_hash + } + + Err(e) => { + print(format!( + "Error: could not query ICP ledger block at height {}:\nCode: {:?}\nMessage: {:?}", + block_height, e.0, e.1 + )); + None + } + }; + + Ok(SubmitTransferResponse { + block_height, + transaction_hash, + }) } } @@ -223,15 +270,21 @@ impl BlockchainApi for InternetComputer { wallet_account: &Account, transfer: &Transfer, ) -> BlockchainApiResult { - let block_height = self + let transfer_response = self .submit_transfer(wallet_account.clone(), transfer.clone()) .await?; Ok(BlockchainTransactionSubmitted { - details: vec![( - ICP_TRANSACTION_SUBMITTED_DETAILS_BLOCK_HEIGHT_KEY.to_string(), - block_height.to_string(), - )], + details: vec![ + ( + TRANSACTION_SUBMITTED_DETAILS_BLOCK_HEIGHT_KEY.to_string(), + transfer_response.block_height.to_string(), + ), + ( + TRANSACTION_SUBMITTED_DETAILS_TRANSACTION_HASH_KEY.to_string(), + transfer_response.transaction_hash.unwrap_or("".to_string()), + ), + ], }) } } diff --git a/canisters/wallet/impl/src/jobs/execute_created_transfers.rs b/canisters/wallet/impl/src/jobs/execute_created_transfers.rs index 9c2072876..3a78919a4 100644 --- a/canisters/wallet/impl/src/jobs/execute_created_transfers.rs +++ b/canisters/wallet/impl/src/jobs/execute_created_transfers.rs @@ -2,8 +2,13 @@ use super::ScheduledJob; use crate::{ core::ic_cdk::api::{print, time}, errors::TransferError, - factories::blockchains::BlockchainApiFactory, - models::{Account, Proposal, ProposalStatus, Transfer, TransferId, TransferStatus}, + factories::blockchains::{ + BlockchainApiFactory, BlockchainTransactionSubmitted, + TRANSACTION_SUBMITTED_DETAILS_TRANSACTION_HASH_KEY, + }, + models::{ + Account, Proposal, ProposalOperation, ProposalStatus, Transfer, TransferId, TransferStatus, + }, repositories::{AccountRepository, ProposalRepository, TransferRepository}, }; use async_trait::async_trait; @@ -99,11 +104,18 @@ impl Job { .iter() .enumerate() .for_each(|(pos, result)| match result { - Ok(transfer) => { + Ok((transfer, details)) => { let mut transfer = transfer.clone(); + + let maybe_transaction_hash = details + .details + .iter() + .find(|(key, _)| key == TRANSACTION_SUBMITTED_DETAILS_TRANSACTION_HASH_KEY) + .map(|(_, value)| value.to_owned()); + transfer.status = TransferStatus::Completed { completed_at: time(), - hash: None, + hash: maybe_transaction_hash, signature: None, }; transfer.last_modification_timestamp = time(); @@ -112,6 +124,13 @@ impl Job { if let Some(proposal) = proposals.get(&transfer.id) { let mut proposal = proposal.clone(); + + if let ProposalOperation::Transfer(transfer_operation) = + &mut proposal.operation + { + transfer_operation.transfer_id = Some(transfer.id); + } + proposal.status = ProposalStatus::Completed { completed_at: time(), }; @@ -155,7 +174,10 @@ impl Job { /// Executes a single transfer. /// /// This function will handle the submission of the transfer to the blockchain. - async fn execute_transfer(&self, transfer: Transfer) -> Result { + async fn execute_transfer( + &self, + transfer: Transfer, + ) -> Result<(Transfer, BlockchainTransactionSubmitted), TransferError> { let account = self .account_repository .get(&Account::key(transfer.from_account)) @@ -171,12 +193,12 @@ impl Job { reason: format!("Failed to build blockchain api: {}", e), })?; - if let Err(error) = blockchain_api.submit_transaction(&account, &transfer).await { - Err(TransferError::ExecutionError { + match blockchain_api.submit_transaction(&account, &transfer).await { + Ok(details) => Ok((transfer, details)), + + Err(error) => Err(TransferError::ExecutionError { reason: error.to_json_string(), - })? + })?, } - - Ok(transfer) } } diff --git a/canisters/wallet/impl/src/mappers/proposal_operation.rs b/canisters/wallet/impl/src/mappers/proposal_operation.rs index 25ace40ff..f5e583b5c 100644 --- a/canisters/wallet/impl/src/mappers/proposal_operation.rs +++ b/canisters/wallet/impl/src/mappers/proposal_operation.rs @@ -48,6 +48,9 @@ impl TransferOperation { name: self.input.network.clone(), }), }, + transfer_id: self + .transfer_id + .map(|id| Uuid::from_bytes(id).hyphenated().to_string()), } } }