Skip to content
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.

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

support generated fields on serverside wallets #1769

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

riccardobl
Copy link
Member

Description

Client-side wallets can transform credentials during initial configuration. Previously, this functionality was also available for server-side wallets, but a recent refactoring broke it. This PR restores it, as it is required for stackernews/stacker.news#1763.

Checklist

Are your changes backwards compatible? Please answer below:
yes

On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:
tested as part of #1763

For frontend changes: Tested on mobile, light and dark mode? Please answer below:

Did you introduce any new environment variables? If so, call them out explicitly here:
no

if (validData) {
data && Object.keys(validData).filter(key => key in data).forEach(key => { data[key] = validData[key] })
settings && Object.keys(validData).filter(key => key in settings).forEach(key => { settings[key] = validData[key] })
}
Copy link
Member Author

@riccardobl riccardobl Dec 27, 2024

Choose a reason for hiding this comment

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

this becomes a function since we need to run it twice:

  • Before testCreateInvoice with skipGenerated=true (because we don't have the generated properties yet)
  • After testCreateInvoice with skipGenerated=false

@@ -779,6 +784,7 @@ async function upsertWallet (
if (testCreateInvoice) {
try {
await testCreateInvoice(data)
await validate({ data, settings, skipGenerated: false })
Copy link
Member Author

Choose a reason for hiding this comment

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

validate again, since now we'll have the generated fields

@@ -63,7 +63,7 @@ export function useWalletConfigurator (wallet) {
throw err
}
} else if (canReceive({ def: wallet.def, config: serverConfig })) {
const transformedConfig = await validateWallet(wallet.def, serverConfig)
const transformedConfig = await validateWallet(wallet.def, serverConfig, { skipGenerated: true })
Copy link
Member Author

Choose a reason for hiding this comment

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

the client needs to skip generated fields for receivers , because they are generated by the testCreateInvoice that runs serverside

@riccardobl riccardobl requested a review from Copilot December 27, 2024 17:00

Choose a reason for hiding this comment

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

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

@@ -72,7 +72,7 @@ function composeWalletSchema (walletDef, serverSide, skipGenerated) {

if (clientOnly && serverSide) {
// For server-side validation, accumulate clientOnly fields as vaultEntries
vaultEntrySchemas[optional ? 'optional' : 'required'].push(vaultEntrySchema(name))
vaultEntrySchemas[(optional || generated) ? 'optional' : 'required'].push(vaultEntrySchema(name))
Copy link
Member Author

Choose a reason for hiding this comment

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

every client-side generated field must be optional for server-side validation, otherwise if the user configures only the receiver part of a send&receiver wallet, the validation will fail because the non optional generated client-side fields are not compiled.

@riccardobl riccardobl marked this pull request as ready for review December 27, 2024 17:08
@huumn
Copy link
Member

huumn commented Dec 28, 2024

Does this fix something broken in production?

@riccardobl
Copy link
Member Author

riccardobl commented Dec 28, 2024

Does this fix something broken in production?

no, it doesn't

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.

2 participants