Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ICRC-3 #128

Merged
merged 46 commits into from
Apr 2, 2024
Merged

ICRC-3 #128

merged 46 commits into from
Apr 2, 2024

Conversation

MarioDfinity
Copy link
Contributor

No description provided.

standards/ICRC-3/README.md Outdated Show resolved Hide resolved
standards/ICRC-3/README.md Outdated Show resolved Hide resolved
@blutooth
Copy link

Really important to be able to query txs by deterministic tx hashes. On ETH it is possible to do this, I hope the IC log contains this functionality, which is crucial for finding transactions.

@sea-snake
Copy link

sea-snake commented Jul 15, 2023

What would be the recommended approach for getting transactions from/to a specific account without fetching the whole transaction history and filtering?

Right now most wallets use rosetta-api.internetcomputer.org to get a list of user transactions, but this isn't on chain and won't work for other icrc1 tokens.

Is this something that will be handled in another ICRC standard?

@MarioDfinity
Copy link
Contributor Author

@sea-snake ICRC-3 is concerned with the transaction log only. You can write a canister that indexes transactions for wallets/transaction explorers consumption based on the ICRC-3 API. For instance, we implemented an index canister for our ICRC Ledger that allows the NNS Dapp to query an Account transactions via get_account_transactions. We are doing the same for the ICP Ledger.

We should discuss about an indexing standard in the L&T WG.

@MarioDfinity
Copy link
Contributor Author

@blutooth ICRC-3 doesn't provide a way to query transactions by transaction hash. This is a good topic for the WG.

@MarioDfinity MarioDfinity changed the title Draft: ICRC-3 ICRC-3 Jul 25, 2023
@MarioDfinity MarioDfinity mentioned this pull request Jul 25, 2023
standards/ICRC-3/README.md Outdated Show resolved Hide resolved
standards/ICRC-3/README.md Outdated Show resolved Hide resolved
standards/ICRC-3/README.md Outdated Show resolved Hide resolved
The [candid](https://github.com/dfinity/candid) format supports sharing information even when the client and the server involved do not have the same schema (see the [Upgrading and subtyping](https://github.com/dfinity/candid/blob/master/spec/Candid.md#upgrading-and-subtyping) section of the candid spec). While this mechanism allows to evolve services and clients
independently without breaking them, it also means that a client may not receive all the information that the server is sending, e.g. in case the client schema lacks some fields that the server schema has.

This loss of information is not an option for `ICRC-3`. The client must receive the same exact data the server sent. For this reason, `ICRC-3` introduces the `Value` type which never changes:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? What problem does this solve?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's explained before.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The candid format supports sharing information even when the client and the server involved do not have the same schema (see the Upgrading and subtyping section of the candid spec). While this mechanism allows to evolve services and clients
independently without breaking them, it also means that a client may not receive all the information that the server is sending, e.g. in case the client schema lacks some fields that the server schema has.

This loss of information is not an option for ICRC-3. The client must receive the same exact data the server sent.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's the text that I commented on. It simply states that it's not acceptable, but it does not explain why.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added the following:

This loss of information is not an option for ICRC-3. The client must receive the same exact data the server sent in order to verify it. Verification is done by hashing the data and checking that the result is consistent with what has been certified by the server.

Is it better?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Why isn't it up to the client if it wants to verify the hashes? If it wants to verify the hashes, it should make sure to have an up-to-date schema. If it only wants certain data, it can use any schema that provides that data. By verifying the hash it can even make sure that its schema is up-to-date so.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The client cannot check that the schema is identical to the one of the canister. There is no guarantee that the schema will be identical even if the client first fetches it from the canister. Candid was built with a different goal in mind: the schema of the server can evolve independently from the client.
The solution to this problem is to have a type that will never change over time so it is guaranteed that clients and canisters will always know the full structure. That's what Value is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it wants to verify the hashes, it should make sure to have an up-to-date schema.

Because it's not a sustainable solution in a decentralized system. If we went this way, releasing a new Ledger feature would immediately break block validation for all existing clients. We don't have a full list of our clients, so we cannot ask them to upgrade in advance.
The main clients of ICRC-3 standard are the on-chain and off-chain indexers, such as the index canister and Rosetta. We want these to continue working correctly with no intervention as we evolve the ledger.

standards/ICRC-3/README.md Outdated Show resolved Hide resolved
standards/ICRC-3/README.md Outdated Show resolved Hide resolved
standards/ICRC-3/README.md Outdated Show resolved Hide resolved
@hpeebles
Copy link

I'd imagine that many apps will store transactions for each user and want to pull in newer transactions when the user loads their transactions page. To do this they'd ideally be able to pass in the latest transaction Id they have stored for the user and request newer transactions than that.
This would be solved by ICRC-3 if we could optionally choose the direction of iteration.
Is this something you have considered adding?
An additional benefit of this is that if you are syncing transactions from a canister you avoid having to make a replicated query to first work out the latest block index.

Copy link
Contributor

@roman-kashitsyn roman-kashitsyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor remark: I suspect many people might get confused about the schema definition language and how it's different from the Candid definitions. We might want to elaborate on the difference between these two languages and move all the schemas in a separate section so the reader doesn't have to switch back and forth between Candid and the Value Schema language.

standards/ICRC-3/README.md Show resolved Hide resolved
standards/ICRC-3/README.md Outdated Show resolved Hide resolved
standards/ICRC-3/README.md Outdated Show resolved Hide resolved
standards/ICRC-3/README.md Outdated Show resolved Hide resolved
standards/ICRC-3/ICRC-3.did Outdated Show resolved Hide resolved
standards/ICRC-3/ICRC-3.did Outdated Show resolved Hide resolved
standards/ICRC-3/ICRC-3.did Show resolved Hide resolved
@dgdg-app
Copy link

dgdg-app commented Mar 9, 2024

Instead of "Value", something more-literal would help casual browsers of the Candid understand what is going on.

Something like "UpgradableResponse".

Copy link
Contributor

@dietersommer dietersommer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made some comments on necessary fixes we should do.

standards/ICRC-3/README.md Show resolved Hide resolved
standards/ICRC-3/README.md Outdated Show resolved Hide resolved
@MarioDfinity MarioDfinity merged commit 7bef69a into main Apr 2, 2024
3 checks passed
@MarioDfinity MarioDfinity deleted the icrc-3 branch April 2, 2024 07:48
gitlab-dfinity pushed a commit to dfinity/ic that referenced this pull request Apr 2, 2024
feat(icrc-ledger-types): add ICRC-3 types

Closes FI-1243

This MR adds [ICRC-3 types](dfinity/ICRC-1#128) to the `icrc-ledger-types` package. The new types have ICRC3 as prefix to distinguish them from the existing ones. The plan is to have both types, the standard and non-standard ones, live together until ICRC-3 is voted and then, once ICRC-3 is voted, replace the old non-standard ones with the new ones and do a breaking change release of `icrc-ledger-types`.

I added "deprecation" comments to the non-standard types with pointers to the counterpart standardized structures. The reason why I don't use the `deprecated` macro is that technically those types are not deprecated, they just don't belong to the `icrc-ledger-types` package because they are not standard. 

Closes FI-1243

See merge request dfinity-lab/public/ic!18329
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.