Skip to content

Conversation

@jchavannes
Copy link
Contributor

Description of Changes

Provide a brief description of the changes you've made.

Linked Issues / Tickets

Reference any related issues or tickets, e.g. "Closes #123".

Testing Procedure

Describe the tests you've added or any testing steps you've taken.

  • I have added new unit tests
  • All tests pass locally
  • I have tested manually in my local environment

Checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have updated CHANGELOG.md with my changes
  • I have run npm run doc and npm run lint one final time before requesting a review
  • I have fixed all linter errors to ensure these changes are compliant with ts-standard
  • I have run npm version patch so that my changes will trigger a new version to be released when they are merged

@jchavannes jchavannes changed the title OPL-491 UTF8 strings instead of Base64 bytes for InternalizeAction prefix/suffix UTF8 strings instead of Base64 bytes for InternalizeAction prefix/suffix Sep 29, 2025
sirdeggen
sirdeggen previously approved these changes Sep 29, 2025
@BraydenLangley
Copy link
Collaborator

Why are we changing derivation prefix/suffix to utf8 vs. base64?

@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
D Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@ty-everett
Copy link
Collaborator

Is this PR still active? None of the checklist items have been completed, and no justification has been given for changing the key derivation. This would be a breaking change with wide-ranging consequences, including changes that impact the behavior of BRC-100 wallets. Unless a compelling justification can be offered, I wouldn't support this breaking change.

@chris-4chain
Copy link
Contributor

I think it might be based on my recent suggestion, so I'd like to offer a bit of clarification. 😅
My intention was not to change the TypeScript version, as that could be an unnecessary breaking change.

My suggestion was to change the DerivationPrefix and DerivationSuffix types from []byte to string only in the Go SDK.

Here's my reasoning:

  1. Consistency with TypeScript - The TS implementation sends the Base64 value over the wire as a string. When this is received on the Go side as a []byte, it causes confusion. We've seen several developers struggle with how to handle this byte slice correctly.
  2. Internally, services like wallet-toolbox-wallet already send these prefixes as a string to wallet-toolbox-storage via JsonRPC. The current []byte type forces us to constantly convert from string to []byte and back again.
  3. The documentation and the TS SDK already use strings. Since the keyID is a string (e.g., "prefix suffix"), using []byte for its components is confusing for new developers. It leads to questions like, "What format should I use here? []byte(derivationPrefix) or Base64Encode(derivationPrefix)?" Using string makes the expected input much clearer.

So, the discussion (in my perspective) concerns only the golang version.

@jchavannes
Copy link
Contributor Author

@chris-4chain updating Golang breaks the wire serialization protocol with TypeScript. That is why changing Golang requires TypeScript change.

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.

5 participants