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.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"

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<ListAccountTransfersResponse, ApiErrorDTO>,) = 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);
 }

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,
 }

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,
   });

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.

canisters/wallet/api/src/transfer.rs

@@ -25,6 +25,7 @@ pub struct TransferOperationDTO {
     pub from_account: Option<AccountDTO>,
     pub network: NetworkDTO,
     pub input: TransferOperationInput,
+    pub transfer_id: Option<UuidDTO>,
 }
 
 #[derive(CandidType, Deserialize, Debug, Clone)]

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"] }

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

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<T> = Result<T, ApiError>;
 
 #[derive(Clone, Debug, Hash)]

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<String>,
+}
+
 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<u64, ApiError> {
+    ) -> Result<SubmitTransferResponse, ApiError> {
         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<BlockchainTransactionSubmitted> {
-        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()),
+                ),
+            ],
         })
     }
 }

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<Transfer, TransferError> {
+    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)
     }
 }