Skip to content

export-and-sign updates #92

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Nov 27, 2025
Merged

export-and-sign updates #92

merged 9 commits into from
Nov 27, 2025

Conversation

@andrewkmin
Copy link
Contributor

@andrewkmin andrewkmin commented Nov 21, 2025 •
edited
Loading

$title

  • Allow for multiple keys in memory; note that this requires an address parameter to be passed in along with injectKeyExportBundle, signMessage, and signTransaction.
    • This is backwards compatible: if it's not passed in, no worries: fall back to defaults
  • Prettier
  • Removing deprecated code around enclaveQuorumPublic being optional. Because this is a new iframe and customers using this will be on recent versions of our SDKs (which have this in export bundles), enclaveQuorumPublic should always be checked.

SDK counterpart: tkhq/sdk#1103

@andrewkmin andrewkmin mentioned this pull request Nov 25, 2025
Merged
3 tasks
@andrewkmin andrewkmin changed the title Andrew/export sign updates export-and-sign updates Nov 25, 2025
@andrewkmin andrewkmin force-pushed the andrew/export-sign-updates branch from 7510849 to 560b91f Compare November 25, 2025 04:25
andrewkmin
andrewkmin commented Nov 25, 2025
View reviewed changes
export-and-sign/src/turnkey-core.js
}

// todo(olivia): throw error if enclave quorum public is null once server changes are deployed
if (enclaveQuorumPublic) {
Copy link
Contributor Author

@andrewkmin andrewkmin Nov 25, 2025

Choose a reason for hiding this comment

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

With this iframe, we always expect clients to be passing in enclaveQuorumPublic. Therefore, we should enforce the expectation that it'll always be available, and check that it matches TURNKEY_SIGNER_ENCLAVE_QUORUM_PUBLIC_KEY

Copy link
Contributor

@moe-dev moe-dev Nov 25, 2025

Choose a reason for hiding this comment

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

Is this a breaking change at all from the sdk side?

Copy link
Contributor Author

@andrewkmin andrewkmin Nov 25, 2025

Choose a reason for hiding this comment

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

Shouldn't be; as of this change https://github.com/tkhq/mono/pull/2806 we've been passing enclaveQuorumPublic around, so it should be required here

andrewkmin
andrewkmin commented Nov 25, 2025
View reviewed changes
export-and-sign/index.test.js
});

it("verifies enclave signature", async () => {
// No "enclaveQuorumPublic" field in the export bundle. Valid signature
Copy link
Contributor Author

@andrewkmin andrewkmin Nov 25, 2025

Choose a reason for hiding this comment

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

We're now requiring enclaveQuorumPublic all the time, so we're removing this unit test/expectation.

@andrewkmin andrewkmin force-pushed the andrew/export-sign-updates branch from 560b91f to 18924c2 Compare November 25, 2025 04:31
andrewkmin
andrewkmin commented Nov 25, 2025
View reviewed changes
andrewkmin
andrewkmin commented Nov 25, 2025
View reviewed changes
andrewkmin
andrewkmin commented Nov 25, 2025
View reviewed changes
export-and-sign/src/event-handlers.js Outdated

// Display only the key
// Display only the key --> this functionality should be deprecated at some point
// TODO: In debug mode, we also now need to be display multiple keys?
Copy link
Contributor Author

@andrewkmin andrewkmin Nov 25, 2025

Choose a reason for hiding this comment

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

this is not critical at this time; note for ourselves as we consider separating this iframe into "invisible" (no HTML/UI elements) vs. "standalone" (with HTML/UI elements for testing/demo purposes)

andrewkmin
andrewkmin commented Nov 25, 2025
View reviewed changes
moe-dev
moe-dev reviewed Nov 25, 2025
View reviewed changes
moe-dev
moe-dev reviewed Nov 25, 2025
View reviewed changes
moe-dev
moe-dev reviewed Nov 25, 2025
View reviewed changes
@andrewkmin andrewkmin force-pushed the andrew/export-sign-updates branch 2 times, most recently from 5c5b22b to 24bf0f7 Compare November 25, 2025 23:42
@andrewkmin andrewkmin marked this pull request as ready for review November 25, 2025 23:42
@andrewkmin andrewkmin force-pushed the andrew/export-sign-updates branch from 24bf0f7 to a4b5643 Compare November 25, 2025 23:43
@andrewkmin andrewkmin force-pushed the andrew/export-sign-updates branch from a4b5643 to 644d026 Compare November 25, 2025 23:44
@andrewkmin andrewkmin requested a review from moe-dev November 25, 2025 23:45
moe-dev
moe-dev previously approved these changes Nov 26, 2025
View reviewed changes
@andrewkmin
updates:
3c5dde8 
- store keys in memory. choosing to go this route over leaving the embedded iframe key even after each decryption
- fix onGetPublicEmbeddedKey()
- throw error if organizationId is not found (for security)
- ensure 0x prefix is not included in private key (can update this later)
@andrewkmin andrewkmin force-pushed the andrew/export-sign-updates branch from 7ff0fb9 to 3c5dde8 Compare November 27, 2025 02:24
@andrewkmin
Copy link
Contributor Author

andrewkmin commented Nov 27, 2025 •
edited
Loading

@moe-dev see 3c5dde8; it corresponds to SDK PR's commit 2eebd7ff3a8435d91ddd4d47e0c3d267d818057d

MasterHW
MasterHW previously approved these changes Nov 27, 2025
View reviewed changes
moeodeh3
moeodeh3 approved these changes Nov 27, 2025
View reviewed changes
Copy link
Contributor

@moeodeh3 moeodeh3 left a comment

Choose a reason for hiding this comment

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

LGTM

@andrewkmin andrewkmin merged commit b8d24f8 into main Nov 27, 2025
9 checks passed
@andrewkmin andrewkmin deleted the andrew/export-sign-updates branch November 27, 2025 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@moeodeh3 moeodeh3 moeodeh3 approved these changes

@MasterHW MasterHW Awaiting requested review from MasterHW

@moe-dev moe-dev Awaiting requested review from moe-dev

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@andrewkmin @moe-dev @MasterHW @moeodeh3