Skip to content

feat: include leader public key when adding funds#105

Open
mfontanini wants to merge 2 commits into
mainfrom
feat/add-funds-leader-key
Open

feat: include leader public key when adding funds#105
mfontanini wants to merge 2 commits into
mainfrom
feat/add-funds-leader-key

Conversation

@mfontanini
Copy link
Copy Markdown

This implements the new leader public key logic for add-funds introduced in NillionNetwork/nilvm#67

@mfontanini mfontanini marked this pull request as ready for review March 13, 2025 16:47
@mfontanini mfontanini requested review from pablojhl and tim-hm and removed request for tim-hm March 13, 2025 16:47
Comment thread client-vms/src/vm/builder.ts Outdated
Comment on lines +199 to +202
if (
!leaderClusterInfo.publicKeys ||
!leaderClusterInfo.publicKeys.authentication
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit, but i think this simplifies to: if (!leaderClusterInfo.publicKeys?.authentication) {}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Dammit
image

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yeah I was just copying that pattern

Comment thread client-vms/src/vm/builder.ts Outdated
.id(user_id)
.signer(signer)
.leader(leader.transport)
.leaderPublicKey(leaderClusterInfo.publicKeys?.authentication)
Copy link
Copy Markdown
Contributor

@tim-hm tim-hm Mar 19, 2025

Choose a reason for hiding this comment

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

nit: is this not type narrowed? eg I think it should be fine as .leaderPublicKey(leaderClusterInfo.publicKeys.authentication)

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.

3 participants