Skip to content

feat(ledgers): FI-1659: fix the generic ICRC-21 message, add FieldsDisplay#5563

Merged
maciejdfinity merged 37 commits intomasterfrom
maciej-hw
Aug 11, 2025
Merged

feat(ledgers): FI-1659: fix the generic ICRC-21 message, add FieldsDisplay#5563
maciejdfinity merged 37 commits intomasterfrom
maciej-hw

Conversation

@maciejdfinity
Copy link
Copy Markdown
Contributor

@maciejdfinity maciejdfinity commented Jun 16, 2025

Fix the generic ICRC-21 message returned by the ledgers to conform to https://github.com/dfinity/wg-identity-authentication/tree/main/topics/ICRC-21/examples, add FieldsDisplay and remove LineDisplay.

CI_OVERRIDE_DIDC_CHECK: the removed LineDisplay shouldn't be used by any clients - it was meant for the Ledger hardware wallet which did not use it in the end either.

@maciejdfinity maciejdfinity changed the title fix generic message fix(ledgers): fix the generic ICRC-21 message Jun 17, 2025
@github-actions github-actions Bot added the fix label Jun 17, 2025
@maciejdfinity maciejdfinity added the CI_OVERRIDE_DIDC_CHECK Skips the backwards compatibility didc check (explain in PR description why) label Jun 19, 2025
Comment thread rs/ledger_suite/tests/sm-tests/src/lib.rs Outdated
@lmuntaner
Copy link
Copy Markdown

@maciejdfinity @mbjorkqvist don't merge this yet.

We encountered a UX issue with Ledger using this format. I'm expecting to have a call with the agency developing the application to better understand the problem and possible solutions.

This means that the format might change.

@maciejdfinity
Copy link
Copy Markdown
Contributor Author

We encountered a UX issue with Ledger using this format. I'm expecting to have a call with the agency developing the application to better understand the problem and possible solutions.

This means that the format might change.

@lmuntaner any news?

@lmuntaner
Copy link
Copy Markdown

@lmuntaner any news?

Yes, we have a confirmation of the new format. The PR to change the standard is here: dfinity/wg-identity-authentication#241

Can you work with that?

@maciejdfinity
Copy link
Copy Markdown
Contributor Author

maciejdfinity commented Aug 5, 2025

@lmuntaner @sea-snake I updated the PR according to changes in dfinity/wg-identity-authentication#241 and dfinity/wg-identity-authentication#243 in this commit: eff5050 IIUC the changes to use Value only affect the FieldsDisplay?

Copy link
Copy Markdown

@lmuntaner lmuntaner left a comment

Choose a reason for hiding this comment

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

Thanks @maciejdfinity !

I added a couple of comments, but yes, it looks good to me.

Comment thread rs/ledger_suite/icp/ledger.did Outdated
Comment thread rs/ledger_suite/icrc1/ledger/ledger.did Outdated
@lmuntaner
Copy link
Copy Markdown

@maciejdfinity do yuo have ledger canisters in mainnet where we could deploy this and test it with the hardware wallet?

@maciejdfinity
Copy link
Copy Markdown
Contributor Author

maciejdfinity commented Aug 5, 2025

@maciejdfinity do yuo have ledger canisters in mainnet where we could deploy this and test it with the hardware wallet?

@lmuntaner we have a TESTICP ledger in Orbit, I could deploy it there: https://dashboard.internetcomputer.org/canister/xafvr-biaaa-aaaai-aql5q-cai Would that work?

edit: I deployed the changes from this PR to the above canister

@maciejdfinity maciejdfinity requested a review from lmuntaner August 5, 2025 15:24
@lmuntaner
Copy link
Copy Markdown

@maciejdfinity do yuo have ledger canisters in mainnet where we could deploy this and test it with the hardware wallet?

@lmuntaner we have a TESTICP ledger in Orbit, I could deploy it there: https://dashboard.internetcomputer.org/canister/xafvr-biaaa-aaaai-aql5q-cai Would that work?

edit: I deployed the changes from this PR to the above canister

Thanks!

I tested it and it works nicely!

WhatsApp Image 2025-08-06 at 11 48 58

Copy link
Copy Markdown

@lmuntaner lmuntaner left a comment

Choose a reason for hiding this comment

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

LGTM!

@maciejdfinity maciejdfinity added this pull request to the merge queue Aug 11, 2025
Merged via the queue into master with commit 3889808 Aug 11, 2025
27 checks passed
@maciejdfinity maciejdfinity deleted the maciej-hw branch August 11, 2025 10:28
kpop-dfinity pushed a commit that referenced this pull request Aug 11, 2025
…splay (#5563)

Fix the generic ICRC-21 message returned by the ledgers to conform to
https://github.com/dfinity/wg-identity-authentication/tree/main/topics/ICRC-21/examples,
add `FieldsDisplay` and remove `LineDisplay`.

CI_OVERRIDE_DIDC_CHECK: the removed `LineDisplay` shouldn't be used by
any clients - it was meant for the Ledger hardware wallet which did not
use it in the end either.

---------

Co-authored-by: Maciej Dfinity <maciej@dfinity.org>
kpop-dfinity pushed a commit that referenced this pull request Aug 13, 2025
…splay (#5563)

Fix the generic ICRC-21 message returned by the ledgers to conform to
https://github.com/dfinity/wg-identity-authentication/tree/main/topics/ICRC-21/examples,
add `FieldsDisplay` and remove `LineDisplay`.

CI_OVERRIDE_DIDC_CHECK: the removed `LineDisplay` shouldn't be used by
any clients - it was meant for the Ledger hardware wallet which did not
use it in the end either.

---------

Co-authored-by: Maciej Dfinity <maciej@dfinity.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI_OVERRIDE_DIDC_CHECK Skips the backwards compatibility didc check (explain in PR description why) feat @finint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants