Skip to content

Conversation

@krpeacock
Copy link
Contributor

@krpeacock krpeacock commented Sep 20, 2023

Description

JSON.stringify(principal) currently uses Uint8Array encoding instead of toString. Adding the toJSON method resolves this

How Has This Been Tested?

new unit tests

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.

@krpeacock krpeacock requested a review from a team as a code owner September 20, 2023 21:11
@github-actions
Copy link
Contributor

github-actions bot commented Sep 20, 2023

size-limit report 📦

Path Size
@dfinity/agent 87.38 KB (+0.01% 🔺)
@dfinity/candid 13.52 KB (+0.05% 🔺)
@dfinity/principal 5.16 KB (+0.12% 🔺)
@dfinity/auth-client 92.55 KB (+0.01% 🔺)
@dfinity/assets 89.96 KB (+0.01% 🔺)
@dfinity/identity 89.82 KB (+0.01% 🔺)
@dfinity/identity-secp256k1 232.51 KB (+0.01% 🔺)

@krpeacock krpeacock merged commit 80ad8fb into main Sep 20, 2023
@krpeacock krpeacock deleted the kyle/principal-json branch September 20, 2023 23:21
* @returns {string} string
*/
public toJSON(): string {
return this.toText();
Copy link
Member

@peterpeterparker peterpeterparker Sep 21, 2023

Choose a reason for hiding this comment

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

Isn't this a bit weak? It means that any string is either a principal or a string which can be quite error prone if you stringify and parse an object that contains string and principals.

In @dfinity/utils we prefix principal to make their encoding and decoding more safe by using JSON reviver and replacer.

https://github.com/dfinity/ic-js/blob/main/packages/utils/src/utils/json.utils.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could serialize into { "__principal__": principal.toText() } instead. The main case I wanted to cover was just ensuring that JSON.stringify() on objects containing principals handles the case gracefully

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.

2 participants