search

dfinity / ICRC

Repository to ICRC proposals
Apache License 2.0
31 stars 5 forks source link

ICRC-84 [WIP]: Deposit and Withdrawal Standard for ICRC-1 tokens #84

Open timohanke opened 5 months ago

timohanke commented 5 months ago

[WIP] ICRC-84: Deposit and Withdrawal Standard for ICRC-1 tokens

Financial service canisters use this standard to allow users to deposit ICRC-1 tokens and withdraw them again. An example for such a service is a DEX.

Tokens

The same service can accept deposits in one or more different ICRC-1 tokens. A token is uniquely identified by the principal of its ICRC-1 ledger.

type Token = principal;

The list of accepted tokens can be queried with the following function.

icrc84_supported_tokens : () -> (vec Token) query;

Amounts

Amounts are specified as nat in the smallest unit of the ICRC-1 token. Decimals do not play a role in the interface.

type Amount = nat;

To get the decimals the user has to query the ICRC-1 ledger.

Users

Users are identified by their principal.

type User = principal;

Deposit accounts

There are two ways for a user to deposit funds to the service. The first one is via direct transfer to a deposit account. The second one is via an allowance, but only if the ICRC-1 ledger supports ICRC-2.

In the direct transfer method, users make deposits into individual deposit accounts which are subaccounts that are derived from the User principal in a deterministic and publicly known way. The derivation works by embedding the principal bytes right-aligned into the 32 subaccount bytes, pre-pending a length byte, and left-padding with zeros.

type Subaccount = blob;

Requirements

The only requirement on the underlying token ledger is the ICRC-1 standard. Since the standard deposit method is based on deposit accounts, not allowances, the ICRC-2 extension is not required. Moreover, as will become clear below, the deposit method is balance-based (as opposed to transaction-based). This means it is sufficient that the service can read the balances in the deposit accounts from the underlying token ledger. It is not required that the service can inspect individual deposit transactions by transaction id, memo or other means. Hence, it is not required that the underlying token ledger provides an indexer, transaction history or archive. In particular, the ICRC-3 extension is not required.

TokenInfo

For each token the service has the following public set of configuration parameters defined. The values may change over time.

type TokenInfo = record {
  deposit_fee : Amount;
  withdrawal_fee : Amount;
  min_deposit : Amount;
  min_withdrawal : Amount;
};

deposit_fee specifies the fee that is deducted each time a deposit is detected and consolidated into the service's main account. The deposit_fee can but does not have to coincide with the transfer fee of the underlying ICRC-1 token. However, the application of the deposit_fee should coincide with actual transfers happening. For example, if the user makes multiple installments into the deposit account and then the service manages to consolidates them all at once into its main account then the deposit_fee should be charged only once. But still, the amount of the deposit_fee can differ from the underlying transfer fee charged by the ledger.

withdrawal_fee specifies the fee that is deducted when the user makes a withdrawal. The withdrawal_fee can but does not have to coincide with the transfer fee of the underlying ICRC-1 token. It is charged for each withdrawal that a user makes and that results in a successful ICRC-1 transfer.

min_deposit is the minimal deposit that is considered valid by the service. Any balance in a deposit account that is below this value is ignored. For example, say for the ICP token a service has defined deposit_fee = 20_000 and min_deposit = 100_000. If the user makes a deposit of exactly 100_000 e8s then 20_000 will be deducted and the user will be credited with 80_000 e8s. The service will empty out the user's deposit account. As a result, the service will take in 90_000 e8s because the ICP ledger's transfer fee is 10_000 e8s. If instead the user had made a deposit of 99,999 then it would have been ignored.

min_deposit must be larger than deposit_fee. For example, if deposit_fee = 20_000 then min_deposit must be at least 20_001.

min_withdrawal is the minimal withdrawal that a user can make. Any withdrawal request below this amount will be denied. min_withdrawal must be larger than withdrawal_fee. For example, say for the ICP token a service has defined withdrawal_fee = 20_000 and min_withdrawal = 100_000. If the user requests a withdrawal of exactly 100_000 e8s then the user will be debited with 100_000 e8s and the service will initiate a transfer of 80_000 e8s to the user. As a result, the service will pay 90_000 e8s because the ICP ledger's transfer fee is 10_000 e8s.

Note: The service will never make transfers of amount 0 on the ICRC-1 ledgers even though ICRC-1 technically allows them. This is true for consolidation of deposits and for withdrawals.

The token info can be queried with the following method.

icrc84_token_info : (Token) -> (TokenInfo) query;

If the specified Token is not supported by the service then the call will throw the async error canister_reject with error message "UnknownToken".

Credits

Credits are tracked by the service on a per-token basis. The unit for credits is the same as the unit of the corresponsing ICRC-1 token. However, credits are of slighly different nature than token balances even though the use the same unit. Credits are virtual and for greater flexibility we allow credits to go negative, hence we use type int.

A user can query his personal credit balance with the following method.

icrc84_credit : (Token) -> (int) query;

If the specified Token is not supported by the service then the call will throw the async error canister_reject with error message "UnknownToken".

Credit balances are private. The above method returns the balance of the caller.

The service is not expected to distinguish non-existing users from existing ones with a credit balance of 0. If the caller is not known to the service, has never used the service before, or has never used the service for the given Token before then the method simply returns a value of zero.

For greater efficiency and to reduce query load, there is a method to obtain a user's credits in all tokens at once.

icrc84_all_credits : () -> (vec record { Token; int }) query;

The returned vector contains all tokens for which the caller has a non-zero credit balance. The tokens with a zero credit balance are stripped from the response.

As before, a non-existing user is handled the same as a user with a zero balance in all tokens. In both cases an empty vector is returned.

Notification

There are two steps required when a user makes a deposit with the direct transfer method:

  1. Make a transfer on the underlying ICRC-1 ledger into the personal deposit account under control of the service.
  2. Notify the service about the fact that a deposit has been made.

Then the service queries the ICRC-1 ledger for the balance in the deposit account and credits the user.

The second step is done via the following method.

  icrc84_notify : (NotifyArg) -> (NotifyResult);

where

type NotifyArg = record {
  token : Token;
};

A call to icrc84_notify notifies the service about a deposit into the deposit account of the caller for the specified token. The service is free to expand this record with additional optional fields to include an action that is to be done with the newly detected deposits.

The result type is as follows.

type NotifyResult = variant {
  Ok : record {
    deposit_inc : Amount;
    credit_inc : Amount;
    credit : int;
  }; 
  Err : variant {
    CallLedgerError : record { message : text };
    NotAvailable : record { message : text };
  };
};

If the specified Token is not supported by the service then the call will throw the async error canister_reject with error message "UnknownToken".

The service will make a downstream call to the underlying ICRC-1 ledger before returning to the user. If the downstream call fails then the variant Err = CallLedgerError is returned. The error message is not specified by this standard but is recommended to describe the async error that actually happened in the downstream call.

The service is not expected to make concurrent downstream calls for the same balance. Hence, if the same caller calls notify twice concurrently for the same Token then the second call will return Err = NotAvailable. This error generally means the notify method is currently blocked for this caller and token, and that it should be retried later. The additional text error message returned with NotAvailable is not specified by this standard.

If the downstream call succeeds then the method will return the Ok record.

The deposit_inc field is the incremental deposit amount that was detected relative to the last known deposit balance. If no new deposit was detected then a zero value is returned.

Calls to notify are not idempotent. If the user makes one deposit transfer and then calls notify twice (with no additional transfer between the two calls to notify) then the first call will return a non-zero deposit_inc value and the second call will return zero.

If the user makes two deposit transfers and then calls notify (with no additional notify call between the two deposit transfers) then notify will return the sum of the two transfer amounts as deposit_inc.

The credit_inc field is the incremental credit amount applied to the user as a result of this call. The value may be lower than deposit_inc due to the application of deposit fees, but does not have to be lower. credit_inc is provided here because the user cannot reliably compute it himself from other data.

The credit field is the absolute credit balance after any newly detected deposit has been credited.

If multiple deposit transactions happened concurrently with calls to notify then the end result may depend on timing. For example, say the ledger fee is 10 and the initial credit balance of the user is 0. If a deposit of 20 tokens is made, then notify is called, then another 20 tokens are deposited and notify is called again then the two notify responses are: { deposit_inc = 20; credit_inc = 10; credit = 10 }, { deposit_inc = 20; credit_inc = 10; credit = 20 }. If the first notify arrives after the second deposit then two responses are: { deposit_inc = 40; credit_inc = 30; credit = 30 }, { deposit_inc = 0; credit_inc = 0; credit = 30 }. In this case the deposit fee is applied only once because the service sees it as one deposit.

The service is free to expand the response record with additional optional fields. For example, if the service has expanded the argument record with a field specifying an action which is done after the notification then it may want to also expand the response record with a field describing the result of that action.

Tracked balance

It was said above that deposit_inc returned by notify is the difference in deposit balance relative to the last known (= "tracked") deposit balance. The tracked deposit balance can be queried with the following method.

icrc84_trackedDeposit : (Token) -> (BalanceResult) query;

If the specified Token is not supported by the service then the call will throw the async error canister_reject with error message "UnknownToken".

Otherwise the method the returns the following type.

type BalanceResult = variant {
  Ok : Amount;
  Err : variant {
    NotAvailable : record { message : text };
  };
};

The Amount returned is the currently known balance that the caller has in the specified Token.

For example, say a deposit flow has been interrupted during the notification step. The user does not know if the attempted call to notify has gone through or not. Then the user can query the ledger to obtain the balance in the deposit account and can query the service to obtain the known deposit balance. If they differ then the user must call notify again.

Of course, the user can call notify directly but the two query calls are considered cheaper and faster. Hence this query method is provided.

If any concurrent downstream calls to the ledger are underway that could affect the returned Amount then the service returns the Err = NotAvailable variant. This indicates to the user to try again later. For example, the downstream call could be a balance query (triggered by notify) or a consolidation transfer that relates to the caller's deposit account for the specified Token.

Deposit

An alternative way to make deposits is via allowances. The user has to set up an allowance for one of its subaccounts with the service's principal as the spender. The user then calls the function

  icrc84_deposit : (DepositArg) -> (DepositResponse);

with the following argument:

type DepositArgs = record {
  token : Token;
  amount : Amount;
  subaccount : opt Subaccount; // subaccount of the caller which has the allowance
};

token is the Token that is being deposited. amount is the amount that is to be drawn from the allowance into the service. Any ledger transfer fees will be added on the user account's side. subaccount is the user's subaccount that carries the allowance where null means the default account.

If successful, the call returns:

type DepositResponse = variant {
  Ok : DepositResult;
  Err : variant {
    AmountBelowMinimum : record {};
    CallLedgerError : record { message : text };
    TransferError : record { message : text }; // insufficient allowance or insufficient funds
  };
};

type DepositResult = record {
  txid : nat;
  credit_inc : Amount;
  credit : int;
};

Possible errors that can occur are:

Withdrawal

The user can initiate a withdrawal with the following method.

icrc84_withdraw : (WithdrawArgs) -> (WithdrawResult);

with

type WithdrawArgs = record {
  to : Account;
  amount : Amount;
  token : Token;
};

The WithdrawArgs record specifies the Token to be withdrawn, the destination account and the Amount to be taken from the caller's credits.

If the specified Token is not supported by the service then the call will throw the async error canister_reject with error message "UnknownToken".

If the specified Subaccount is not 32 bytes long then the call will throw the async error canister_reject with error message "InvalidSubaccount".

Otherwise, the following result type is returned.

type WithdrawResult = variant {
  Ok : record {
    txid : nat;
    amount : Amount;
  };
  Err : variant {
    InsufficientCredit : record {};
    AmountBelowMinimum : record {};
    CallLedgerError : record { message : text };
  };
};

If the user's credit is below the requested Amount then Err = InsufficientCredit is returned.

If the requested Amount is smaller than the Token parameter min_withdrawal then Err = AmountBelowMinimum is returned.

If the downstream call to the ICRC-1 ledger fails with an async error then Err = CallLedgerError is returned. The accompanying text message should indicate the actual async error that happened.

Otherwise the Ok variant is returned. It contains the txid on the underlying ICRC-1 ledger of the withdrawal transfer. It contains the Amount that was actually received by the user. In general, this Amount will differ from the requested amount because withdrawal_fee was deducted.

FAQ

Why is notify access-controlled?

Notify is not idempotent in its return value. If someone else can call notify for us then we could miss an incremental value.

Notify calls are expensive for the service because of the downstream inter-canister call that they trigger. Restricting the caller makes it easier to control or charge for that cost.

Why is the credit balance access-controlled?

Deposits are publicly visible on the ICRC-1 ledger. Any observer can conclude from those deposit transactions to corresponding incoming credits for the user. But from there on further changes to the credit balance, increase or decrease, depend on the usage of the service by the user. For example, in a DEX the credit changes would correspond to bids placed or trades executed. We do not want to leak that information.

Why does notify use a balance-based approach, not transaction-based?

The transaction-based approach would mean that the user "claims" a specific deposit transaction where the transaction is specified by txid and is bound to the user by memo. The advantage is that individual deposit accounts can be avoided, hence the consolidation step is not needed which saves fees.

The disadvantages are:

We prefer the approach that requires less state. It makes the service leaner and easier to handle upgrades.

What are the benfits of using notify vs allowances?

Allowances are simpler to process for the service. Overall transaction fees are lower if an allowance is used for multiple deposits.

But allowances do not always work, for example if

skilesare commented 5 months ago

Very happy to see this as I've almost written it a couple dozen times. Lots of great thoughts.

A few thoughts:

  1. Could we make Token: 
type Token = record {
    canister: Principal;
    asset_id: opt nat;
};

Why? For ICRC1/2 tokens the id will always be null, but if there are future multi-token canisters(much like you designed for HPL). Adding this now will make it easier to use in the future without refactoring or having to issue a new ICRC.

  1. icrc84_supported_tokens and icrc84_all_credits could benefit from adding the paging syntax used in ICRC-7. (prev: opt Item, take: opt nat) which will let the standard support responses that go over 2MB. Maybe seems silly, but I think I saw that Solona had something crazy like 475,000 tokens generated for it over the course of a couple of months. This syntax allows for paginating but may require the statement that the items should be returned in some deterministic order.

  2. notify - I understand the reasoning that you want to not have to worry with icrc3 and tracking burned blocks, but I have concerns about the alternative where an account may become 'locked' by the lock not being released if something goes haywire. I'm sure we can find a good programmatic solution, maybe with a time-out or something, but curious if you have run across that or have any concerns.

  3. ICRC-10...need to reference the ICRC-10 key to use for support detection. https://github.com/dfinity/ICRC/blob/main/ICRCs/ICRC-10/ICRC-10.md

  4. Secrets. I don't like 'em. :) It would be nice to have either a separate ICRC for 84 block types for ICRC-3, or perhaps we can put them here and mark their use as optional?

timohanke commented 5 months ago

@skilesare Thanks for reviewing this and thanks for your interesting comments. I would love to discuss them further.

Re 1. Any future multi-token ledger, HPL or other, will not comply with ICRC-1 because the ICRC-1 itself does not allow to specify an asset id or similar in its interface. So we have to decide if this standard (icrc-84) is supposed to be icrc-1 specific or if its more general. I thought for a while to make it more general and at that time wanted to specify the token in the form of a variant like #icrc1 : principal which would leave room for future extensions to other tokens. But then I thought that all the other functions for deposit/withdrawal, both from an interface perspective as well as from a behaviour perspective, are kind of icrc-1 specific. Without knowing how a future multi-token ledger would work and how its interface looks like it would be hard to design this present proposal in a future proof way. So I decided to drop that path and stay icrc-1 specific with the goal of staying simple and not over-designing it. That being said I would be very interested in people's opinion on this question in general.

At the same time, it is not wise to forego easy ways that can buy us extendability. So for the token specification we would either make it a variant (like #icrc1 : principal) or, as you propose, a record. Along the lines of your proposal the easiest would to just wrap the token principal in a record like record { ledger : principal }. We don't even have to specify the second field now because it can just be added in later. If the new field is opt then candid subtyping will automatically set it to null if the caller does not specify it. I would propose as a first step to do this wrapping.

timohanke commented 5 months ago

Re. 2 for icrc84_supported_tokens. I was expecting this question to come up during the discussion. The question is even if that many tokens on ICP if a single service would necessarily support that many. With 2MB (or is it 3MB for external query calls?) at say 200 bytes per token we can support up to 10k tokens in a service. Probably the answer will be yes we need pagination.

As for the order, what exactly is the requirement? Is it only that the order is deterministic (as in for example alphabetical order)? Or is it append-only so that newly added tokens do not change the order of older tokens (as in for example chronological order, the order in which tokens are added to the service)? I would expect that for pagination to be useful for the caller the order has to be the chronological one.

I am wondering what requirements we create for the implementation, hence the question. For example, if the supported tokens are stored in a tree then the type of tree has to be chosen so that the order matches the iteration order when iterating through the tree entries. Or there need to be two data structures, a tree for lookup and a vector for enumeration. It is not necessarily a problem, we just have to be aware of it when we add pagination.

And we have to be aware that we may be exposing an implementation detail. For example a token principal may now have an "index" in the sequence which also uniquely identifies it (in this service at least).

timohanke commented 5 months ago

Re. 2 for icrc84_all_credits. Similar questions apply as above with the some additional considerations:

There is also a different approach possible which is an alternative approach to pagination. It is if the caller provides, in an argument, the list of tokens that he wants to query. Then the caller can make sure the list isn't too long and can split a long list over two calls. It is also more flexible if the user interested in an update of just a few balances, not all. However, it cannot be used if the caller has forgotten which tokens he owns and wants to know all balances that he owns. So there is a trade-off.

timohanke commented 5 months ago

Re. 3 (notify/lock). Are you saying that any implementation will necessarily require locks? (probably true, just asking) Can you elaborate more on this issue? For example are you afraid of a lock not getting released because of bugs? Or because of calls not responding? By IC issues or by a token ledger being malicious/buggy? What would be the impact on the services if a lock does not get released? Does an approach based on transaction history really avoid all these problems? I mean a response not coming back would block upgrades even with that approach.

timohanke commented 5 months ago

Re 4. What do you mean by ICRC-10 key? I see the ICRC-10 record has a name and a url and the name is given (must be "ICRC-84"). Do you mean the url?

timohanke commented 5 months ago

Re. 5. Can you elaborate more please? ICRC-3 is for "events" not for balances, right? What events are secret here that you would like to see ICRC-3 block types for?

skilesare commented 5 months ago

Re 1.

record { ledger : principal }

I think this might make the most sense, but using a variant might be more complete. I'd be open to either. Apparently, the new motoko compiler will do way with having to null the Variants to make them backwards and forwards compatible, so that might be the easiest.

I'd suggest #icrc1: Principal. Later if we have something like #icrcHPL: record { ledger: Principal;asset_id: Nat} it will be expandable and explicit.

Not to confuse too many issues, but if we did actually do #icrc1 and #icrc2 we could specifically reject icrc2 notify attempts and/or icrc1 deposit attempts. I guess those will fail gracefully anyway, but pushing people down the transfer from route probably makes the code cleaner and avoids locks if it is available.(Just thinking out loud here....I think just having #icrc1 is fine for now.

skilesare commented 5 months ago

Re. 2

For example, if the supported tokens are stored in a tree then the type of tree has to be chosen so that the order matches the iteration order when iterating through the tree entries. Or there need to be two data structures, a tree for lookup and a vector for enumeration. It is not necessarily a problem, we just have to be aware of it when we add pagination.

This pops up a ton when discussing these standards and I've had to go back and retrofit a few indexes into some things that I didn't really want to do, but we probably want consistent, deterministic ordering that makes sense to both the publisher and consumer. Supported tokens seems like it would be either alpha by ledger canister id or binary order by canister ID....the Token objects we have don't have a symbol saved in the data, or I would say that makes the most sense. Likely not a huge deal here at the moment....I think it may be safe to says MUST return items in a "consistent order" and leave the exact order up to the implementation.

Re. 2 and icrc84_all_credits

I think you just want to return items with a non-0 credit balance, and again, using a "consistent order". You just want to make sure that the consumer is able to get all the items by going from front to back. Obviously it is possible for something to get inserted up at the front of the queue while querying if there are bunch of pages, but I don't think there is much that we can do about that.

skilesare commented 5 months ago

Re. 3

Locks are probably the answer for this if you don't want to do burned blocks. Basically, my brain has this block that occurs when I start thinking about all the ways the IC could fail and squaring it with all the tech there to make sure that doesn't happen(and that is before I get stressed out about my own bugs). Of course, a canister can be in the middle of a upgrade, or a subnet could be really slow, or the output queue could get full and time out after five minutes or something.

I was doing an implementation of withdraw and I avoided a lock by burning the balance before trying to send out the tokens and then minting them back if something fails. I think this works well and avoids a double withdraw.

Notify is a bit more problematic as you don't want rapid requests getting into a race condition. Probably better to just put some kind of lock on that account until you get some response back and have a clean up timer that runs once a day just in case something odd happens. Again...just thinking out loud here. Ideally I'd like to just not have notify and force everyone through approvals and in the contract I'm building now it always uses ICP so I know that is there.

skilesare commented 5 months ago

Re 4 ICRC-10

I'm just saying that the standard should specifically say that an ICRC-10 record indicating support for icrc-84 should be added so that wallets and other services can detect that it is supported.

Basically just copying this section and changing it to icrc84 https://github.com/dfinity/ICRC/blob/main/ICRCs/ICRC-7/ICRC-7.md#icrc10_supported_standards

skilesare commented 5 months ago

Re. 5

I'm just posing the question that if an ICRC-84 service doesn't have any reason to keep credit balances a secret and wants to publish a complete transaction log of all activity on the server, it would be nice to have the ICRC-3 block schemas defined somewhere.

For example, for ICRC-72 we pulled the block schemas out and moved them to ICRC-83 as there are likely many event systems that won't want to log every state change to the service, but for those that do, we provide the schemas: https://github.com/icdevs/ICEventsWG/blob/main/Meetings/20240529/icrc83draft.md (this is still an early draft)

skilesare commented 5 months ago

A few implementation thoughts:

An ICRC-84 service does not have subaccount. This means that if a canister wants to use it, it will need to co-mingle user funds in the same set of credits without a way to distinguish who's credits are who's on a per token basis.

Use case: I have written a bot that monitors prices across the IC and I offer a menu of possible actions that users can configure their actions to draw from(see defi vectors https://github.com/Neutrinomic/defivectors). In this case, the vector canister would need to keep all funds in a default account and track them locally.

This may be by design, but I thought it was worth stating out loud.

I was trying to implement it for use in a simple 'wrapped token' contract that tracks cycles for a tokenomics use case and then lets users withdraw them to the new cycles ledger. I didn't need deposits or notify because the balances are pushed in from the behavior of other users. Once the balances were there though, I had them going to standard ICRC1 type accounts.....this meant that if _credits and _trackedDeposit only work for the default account. Not a huge deal, but maybe there is a solution for this?

Derived canister IDs would help for this if there were services that wanted to use this kind of system from one canister while managing many user's accounts.

For withdrawal, I have the workaround that it will withdraw from the same icrc1 account as specified in the to field, but this obviously restricts some usefulness as you can't direct your funds to a different account.

I have a bit of an alternative take on this kind of multi token deposit system that I've been working on and it is now about third down on my todo list...I wanted to take a quick turn at implementation before I released much about it, but perhaps we can compare notes. It has the same kind of restriction that you lose multi-subaccount functionality

timohanke commented 5 months ago

I'd suggest #icrc1: Principal. Later if we have something like #icrcHPL: record { ledger: Principal;asset_id: Nat} it will be expandable and explicit.

It is not that easy. Just changing the Token type from principal to variant { ledger : principal } is not enough. For example, look at the argument type for the deposit function:

type DepositArgs = record {
  token : Token;
  amount : Amount;
  subaccount : opt Subaccount; // subaccount of the caller which has the allowance
};

A different ledger (take for example HPL) may not have subaccounts but may need some other fields to specify a pull transfer. The same holds for the argument of the withdrawal function.

We would have to wrap the whole argument list into a variant with case #icrc1, not just the Token. In summary we have to take all functions defined by this standard and for each of them wrap the entire argument list into a variant { icrc1 : record {..} }. Which raises the question why not make a new standard for each type of ledger?

timohanke commented 5 months ago

Re. pagination for supported_tokens. At any point in time, and between calls, new supported tokens can get added. Let's suppose also that supported tokens can get removed. On the other hand, adding or removing supported tokens is infrequent enough so that the probability of it happening between subsequent paginated calls from the same user is low. So what if supported_tokens returns the timestamp from when the internal list was last updated? The user can then interpret the situation. If the user gets all pages back and they all contain the same timestamp then he knows the pages are consistent. If the timestamps differ then the user has to start querying all pages again.

Without such a mechanism the user could accidentally miss some supported tokens.

timohanke commented 5 months ago

Ideally I'd like to just not have notify and force everyone through approvals and in the contract I'm building now it always uses ICP so I know that is there.

Approvals require the user to make this additional step of setting up an allowance. It's an additional friction for someone who quickly wants to try out the service for the first time. He cannot deposit from an exchange. And most wallets don't support setting allowances yet, or do they? Moreover, you need to properly use an allowance with a subaccount built into it (not just a default allowance), for otherwise the service will just credit the funds to the owner of the allowance. That doesn't help the user at all because the service will credit it to the user's wallet principal. But the user needs it credited to the principal that he is using in this service frontend. Is there even any wallet out there at the moment that supports non-default allowances (with a subaccount built into them)?

I think notify will be the go-to way for most users.

timohanke commented 5 months ago

I'm just posing the question that if an ICRC-84 service doesn't have any reason to keep credit balances a secret and wants to publish a complete transaction log of all activity on the server, it would be nice to have the ICRC-3 block schemas defined somewhere.

So in our case that would be any changes to the internal credit balances, right? If this standard is about deposit and withdrawal then I think it should not be concerned about what services do internally with credits. If you want to log credit changes then that should be its own standard. That's a feature that is independent. It can even exists for a service that does not have deposits and withdrawals (but credits that are based on something else).

timohanke commented 5 months ago

An ICRC-84 service does not have subaccount. This means that if a canister wants to use it, it will need to co-mingle user funds in the same set of credits without a way to distinguish who's credits are who's on a per token basis.

The scenario is where a service canister which has multiple users is the user of an icrc-84 compliant other service and your are comparing: a) the first service has one credit balance in the second service and internally tracks how this one credit maps to the different own users, b) the first service maintains multiple credit balances in the second service (via subaccounts) and maps them to his own users.

From a security perspective there seems to be no difference because the first service is always in full control of the credit. Maybe if the first service loses its data in an upgrade then the individual user credits are still maintained in the second service, is that the benefit?

The price we would pay for b) is that now every icrc84 service is forced to support credit subaccounts. This comes with implementation burden and resource burden. I doubt it is worth it for the gain.

With the current interface any icrc84 can already offer subaccounts on an opt-in basis. To give some context, the notify interface is already designed so that each service can define its own "actions" that a caller to notify can specify, so that the caller can make a single call that effectively becomes "notify_and_X" where X is some action (same for deposit_and_X, etc.). The original idea was to do save a second call and therefore save latency. But you can also use it to bind a specific deposit to a specific action. As an example, if the service is a minter, then notify_and_mint means I can make a deposit and mint a wrapped token in one call, saving latency for the user. For your use case, the service can offer notify_and_segregate which would mean to assign the new credit to a certain subaccount.

The "action" can simply be added as a new field to the NotifyArg or DepositArg record. For example:

type NotifyArg = record {
  token : Token;
  action : opt principal;
};

Services can just add their custom action to optional fields in the argument record. Action-specific results can be added as new fields to the corresponding response record.

skilesare commented 4 months ago

Which raises the question why not make a new standard for each type of ledger?

Hmm...I'd say yes if things are that much different, but for example, Depositing to ICRC-7 could use this...and a couple other's that I've been looking at still use ICRC-1.Account formats....so I think would could at least expand it to those without having a new standard. In fact, adding ICRC-7 should be fairly easy, except that the possible values for credits are only 0 or 1.

skilesare commented 4 months ago

So what if supported_tokens returns the timestamp from when the internal list was last updated?

This is a great idea and something I wish we'd thought of for ICRC-7 etc.😬

skilesare commented 4 months ago

I think notify will be the go-to way for most users.

Good points here...I hope we'll see approve showing up in more wallets, etc....but you are right that it can be more burdensome if the user is not using a dapp to do this procedure.

skilesare commented 4 months ago

That's a feature that is independent. It can even exists for a service that does not have deposits and withdrawals (but credits that are based on something else).

At some point I'm going to have to sit down and make a list of all these more generic block types...This is a good thought here and I'll put it on my list.

skilesare commented 4 months ago

For your use case, the service can offer notify_and_segregate which would mean to assign the new credit to a certain subaccount.

Very nice...I like it! Thanks for the feedback.

timohanke commented 4 months ago

Depositing to ICRC-7 could use this...and a couple other's that I've been looking at still use ICRC-1.Account formats....so I think would could at least expand it to those without having a new standard. In fact, adding ICRC-7 should be fairly easy, except that the possible values for credits are only 0 or 1.

Makes sense. Just double-checking: the single change of making type Token = record { ledger : principal } is enough to later phase in ICRC-7 support, right? Or do we need anything else? As you said credit values are 0 or 1 so we don't need to change the Amount type. Anything else that I am missing here?

timohanke commented 4 months ago

So what if supported_tokens returns the timestamp from when the internal list was last updated?

This is a great idea and something I wish we'd thought of for ICRC-7 etc.😬

Pagination will probably require us to provide a separate function that returns the current number of items. Theoretically the caller can make sequential queries until he receives back a page of shorter length than the take argument, at which point he knows he has reached the end. However, in practice the caller likely wants to know the number of item ahead of time. For example, he could be allocating an Array to store them. Or he might want to query for all pages concurrently rather than sequentially.