Skip to content

Conversation

@q-uint
Copy link
Contributor

@q-uint q-uint commented Oct 12, 2025

Description

ICRC1 account textual representations require both CRC32 (already exposed) and Base32 encoding. This PR exposes the existing internal Base32 implementation, allowing users to display ICRC1 accounts without adding any new dependencies.

Details:

  • Exposes the internal Base32 encoder for external use.
  • Avoids introducing additional third-party libraries.
  • Complements the already-exposed CRC32 functionality needed for ICRC1 account representation.

Motivation

Library users can now generate and display ICRC1 account strings directly, using only @icp-sdk/core/principal.

Checklist:

  • My changes follow the guidelines in CONTRIBUTING.md.
  • The title of this PR complies with Conventional Commits.
  • I have edited the CHANGELOG accordingly.
  • I have made corresponding changes to the documentation.

@ilbertt 😉

@q-uint q-uint requested a review from a team as a code owner October 12, 2025 08:52
@peterpeterparker
Copy link
Member

peterpeterparker commented Oct 12, 2025

For what it's worth, the functions encodeBase32 and decodeBase32 are available in @dfinity/utils.

Those are e.g. used in @dfinity/ledger-icrc for encoding and decoding textual representation of ICRC account.

@ilbertt
Copy link
Member

ilbertt commented Oct 13, 2025

@q-uint how often does it happen that your only dependency is @icp-sdk/core when dealing with ICRC? Isn't it the case that most of the projects already have @dfinity/ledger-icrc as dependency, and hence @dfinity/utils?
Can you provide an example of how you would use this fix?

I feel like the principal submodule is not the right place from where to export those utility functions. Who needs those functions shouldn't search for them into the principal submodule in my opinion.

@q-uint
Copy link
Contributor Author

q-uint commented Oct 13, 2025

It seems like this is more of a personal preference. With bindgen, in my opinion, you don’t need all these dependencies, and it’s generally not a good practice, imo, to rely on them unnecessarily.

My current dependency list is:

"dependencies": {
  "@icp-sdk/core": "^4.0.1"
}

I prefer to keep it this way.


Feel free to close this.

@q-uint
Copy link
Contributor Author

q-uint commented Oct 13, 2025

I feel like the principal submodule is not the right place from where to export those utility functions.

Why do we expose getCrc32?

@ilbertt
Copy link
Member

ilbertt commented Oct 13, 2025

Why do we expose getCrc32?

That's true, we added it upon request in #1077. So it makes sense to also re-export base32 utils.

Would you like to add a base32.test.ts file with some unit tests? So that we export functions that have some test coverage.

@ilbertt
Copy link
Member

ilbertt commented Oct 13, 2025

@q-uint I've opened #1160 to rename those functions. Let's merge that first

ilbertt added a commit that referenced this pull request Oct 13, 2025
Not a breaking change since these functions are not exposed. In preparation of #1159.
@ilbertt
Copy link
Member

ilbertt commented Oct 13, 2025

@q-uint you can update your branch now

ilbertt
ilbertt previously approved these changes Oct 13, 2025
Copy link
Member

@ilbertt ilbertt left a comment

Choose a reason for hiding this comment

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

@q-uint can you update the changelog as well?

## [Unreleased]

+ - feat(principal): export the `base32Encode` and `base32Decode` functions

## [4.0.5] - 2025-09-30

ilbertt
ilbertt previously approved these changes Oct 13, 2025
@github-actions github-actions bot dismissed stale reviews from ilbertt and ilbertt October 13, 2025 14:00

Review dismissed by automation script.

ilbertt
ilbertt previously approved these changes Oct 13, 2025
@github-actions github-actions bot dismissed ilbertt’s stale review October 13, 2025 14:54

Review dismissed by automation script.

@ilbertt ilbertt changed the title feat: expose base32 feat(principal): export the base32Encode and base32Decode functions Oct 13, 2025
@ilbertt ilbertt merged commit 7728222 into dfinity:main Oct 13, 2025
47 checks passed
@ilbertt
Copy link
Member

ilbertt commented Oct 13, 2025

Merged! I'll release 4.1.0 soon.

Thanks a lot for the contribution!

@ilbertt
Copy link
Member

ilbertt commented Oct 13, 2025

@icp-sdk/core@4.1.0 is out with your contribution! Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants