Extend cycles minting canister functionality

I want to propose to add the following functionality to the cycles minting canister

  1. a refund method that allows a user to reclaim ICP (for example when a transaction is send with the wrong memo / subaccount)
  2. Add ICRC support (icrc1_memo)

Maybe @icme has some other suggestions as wel.

@daniel-wong @0x5279616e

2 Likes

The Governance team at DFINITY has resolved to do a couple of things:

  1. Also look at ICRC-1 memo.
  2. When the memo is not one of the special values, refund automatically.

If these measures do not address the problem(s), please, lmk. Otherwise, expect these features to be forthcoming.

Your continued support of the Internet Computer is greatly appreciated!

6 Likes

Thanks! This would already be really helpful in preventing wrong transactions and making the ICRC2 flow partially possible!

wen publish these measures

Not sure. Let’s pencil in January?

I was thinking little endian should be assumed, because that’s what WASM does. Hope this works for everyone. If not, please lmk. TY!

The first feature (icrc1) went out on Monday. (implementation)

I think I will have time to work on the second one (auto refund) this week. Barring unforeseen issues, I estimate that it will not be so difficult.

3 Likes

The second of the proposed features (auto refund) will very very likely be out on Monday.

To be precise, the code is in the master branch. The feature was implemented here, and enabled shortly after that here.

Bon appetit! We really hope you enjoy these features.

As always, your continued support of the Internet Computer is greatly appreciated!

3 Likes

I was trying to implement the cycle topup flow based on doing a icrc2_approve on the frontend and an icrc2_transfer_from on the canister to handle the topup. While the icrc2_transfer_from uses the icrc1_memo field as shown in this transaction it throws the following error

"Refunded: block_index: Some(20385278), reason: "Memo (0x000000) in the incoming ICP transfer does not correspond to any of the operations that the Cycles Minting canister offers.""

I tried a bunch of things like;

  • setting the memo on the icrc2_approve
  • removing the default prefixed 0’s from the memo like shown on this transaction
  • I also thought it might be do to the difference between the icrc_ledger_types memo and the ic_ledger_types memo but so far no luck

So this is the code i’m working with, its based on the working code i had for just a regular transfer, what am i doing wrong? Maybe only icrc1_transfer’s are allowed?

static async approve(to: Icrc1Account, amount: bigint) {
		return this.actor(false).icrc2_approve({
			amount,
			from_subaccount: [],
			spender: {
				owner: to.owner,
				subaccount: to.subaccount
			},
			fee: [],
			memo: [],
			created_at_time: [],
			expected_allowance: [],
			expires_at: [BigInt(Date.now() * 1_000_000 + 5 * 60 * 1_000_000_000)]
		});
	}
pub async fn top_up_cycles_by_approve(
    icp_amount: u64,
    from: Principal,
    canister: Principal,
) -> CanisterResult<u64> {
    let amount = Tokens::from_e8s(icp_amount - ICP_TRANSACTION_FEE);

    let args = TransferFromArgs {
     // memo: Some(Memo::from(0x50555054), // 
        memo: Some(Memo::from(vec![0x50, 0x55, 0x50, 0x54])), // removes prefixed 0's attempt
        amount: amount.e8s().into(),
        fee: None,
        to: Account {
            owner: MAINNET_CYCLES_MINTING_CANISTER_ID,
            subaccount: Some(Subaccount::from(canister).0),
        },
        created_at_time: None,
        spender_subaccount: Some(Subaccount::from(from).0),
        from: Account {
            owner: from,
            subaccount: None,
        },
    };

    let (result,): (Result<Nat, TransferFromError>,) =
        ic_cdk::call(MAINNET_LEDGER_CANISTER_ID, "icrc2_transfer_from", (args,))
            .await
            .map_err(|err| {
                ApiError::external_service_error(&format!("{:?}", err))
                    .add_method_name("top_up_cycles_by_approve")
                    .add_source("toolkit_utils")
            })?;

    match result {
        Ok(block_index) => Ok(nat_to_u64(&block_index)),
        Err(err) => Err(ApiError::external_service_error(&format!("{:?}", err))
            .add_method_name("top_up_cycles_by_approve")
            .add_source("toolkit_utils")),
    }
}
pub async fn notify_top_up_cycles(block_index: u64, canister_id: Principal) -> CanisterResult<Nat> {
    let method_name = "notify_top_up_cycles";
    let source = "toolkit_utils";

    match CyclesMintingService(MAINNET_CYCLES_MINTING_CANISTER_ID)
        .notify_top_up(NotifyTopUpArg {
            block_index,
            canister_id,
        })
        .await
    {
        Ok((result,)) => match result {
            NotifyTopUpResult::Ok(cycles) => Ok(cycles),
            NotifyTopUpResult::Err(err) => match err {
                NotifyError::Refunded {
                    block_index,
                    reason,
                } => Err(ApiError::external_service_error(&format!(
                    "Refunded: block_index: {:?}, reason: {:?}",
                    block_index, reason
                ))
                .add_method_name(method_name)
                .add_source(source)),
                NotifyError::InvalidTransaction(value) => Err(ApiError::external_service_error(
                    &format!("InvalidTransaction: {:?}", value),
                )
                .add_method_name(method_name)
                .add_source(source)),
                NotifyError::Other {
                    error_message,
                    error_code,
                } => Err(ApiError::external_service_error(&format!(
                    "Other: error_message: {}, error_code: {:?}",
                    error_message, error_code
                ))
                .add_method_name(method_name)
                .add_source(source)),
                NotifyError::Processing => Err(ApiError::external_service_error("Processing")),
                NotifyError::TransactionTooOld(value) => Err(ApiError::external_service_error(
                    &format!("TransactionTooOld: {:?}", value),
                )
                .add_method_name(method_name)
                .add_source(source)),
            },
        },
        Err((_, err)) => Err(ApiError::external_service_error(&err)
            .add_method_name(method_name)
            .add_source(source)),
    }
}
1 Like

Hi, rem.codes. Sorry it’s not working for you.

The magic memo you need is

  • 84 = 0x54 = ‘T’
  • 80 = 0x50 = ‘P’
  • 85 = 0x55 = ‘U’
  • 80 = 0x50 = ‘P’
  • 0
  • 0
  • 0
  • 0

Whereas, your transaction’s icrc1_memo is “PUPT”. Therefore, IIUC, you need to

  1. reverse (“TPUP”), then
  2. pad with four 0’s

(Rationale: CMC translates from a blob to an unsigned 64-bit integer, and uses little endian convention (like WASM).)

Hope this works for you. LMK if you still have problems.

Reference(s)

Here is precisely how CMC interprets icrc1_memo.

Doesn’t the old memo use big endian? I seem to remember being huddled in the floor in a puddle of tears when I realized I needed to be encoding PUPT instead of the expected TPUP.

This is confusing to have in two different formats AND two different orders. I guess good docs will help…just give me all the 32 byte arrays I need for each icrc action paired with the Nat32s I need with transfer in a nice neat table in the reader and online docs. Paging @Jessie

We might be confused about what “the old memo” is, but before icrc1, the memo was not a blob. It was (effectively) just a nat64. Therefore, there is no endianness there.

However, what I later found is that our Rust libraries have a conversion from u64 to ircr1 Memo that uses big endian. I did not know about that when I implemented this feature.

IMHO, that conversion (from u64 to icrc1 Memo) should not exist, because from ledger’s point of view, memo is an opaque blob. It has no idea that a blob of length 8 contains an unsigned integer, and therefore, it cannot know what endianness convention is in use between the sender and receiver of the ICP.

Instead, it is up to the sender and receiver to agree on a protocol for how memo is used. In this case, the receiver is CMC, and the protocol is specified where I said “I was thinking little endian should be assumed” earlier in this thread. FWIW, I went with little endian, because that is what WASM uses (and WASM uses little endian, because this is what hardware most commonly supports).

ofc, it is very unfortunate that the Rust conversion from u64 to icrc1 Memo uses big endian while CMC’s convention is little endian. This is admittedly extremely confusing to clients.

One thing CMC can do is extend its protocol to also accept big endian. I am mildly against this, but if we find that confusion is rife, then, I’d agree that also accepting big endian would be worth it. It would be fairly easy to implement; it would probably just require a few additional lines here.

Hmm…I think the nat 64 was the the term utf8 encoded and then converted to a nat 64.

(But this is ultimately not important).

Your idea may be helpful.