feat(awm): backup awm client configurations#196
Open
margueriteblair wants to merge 1 commit intomasterfrom
Open
feat(awm): backup awm client configurations#196margueriteblair wants to merge 1 commit intomasterfrom
margueriteblair wants to merge 1 commit intomasterfrom
Conversation
margueriteblair
force-pushed
the
margieblair/wcn-362-akm-support-separate-awm-instances-for-user-and-backup-key
branch
2 times, most recently
from
f94534e to
059e29d
Compare
margueriteblair
changed the title
margueriteblair
force-pushed
the
margieblair/wcn-362-akm-support-separate-awm-instances-for-user-and-backup-key
branch
from
e39a781 to
9e0531d
Compare
Author
margueriteblair
changed the title
pranavjain97
requested changes
May 4, 2026
Comment on lines
+502
to
+547
| // Handle cert loading for backup AWM (only when backup URL is configured) | ||
| if (config.advancedWalletManagerBackupUrl) { | ||
| if (config.awmBackupServerCaCertPath) { | ||
| try { | ||
| if (fs.existsSync(config.awmBackupServerCaCertPath)) { | ||
| config.awmBackupServerCaCert = fs.readFileSync(config.awmBackupServerCaCertPath, 'utf-8'); | ||
| logger.info( | ||
| `✓ AWM backup server CA certificate loaded from file: ${config.awmBackupServerCaCertPath?.substring( | ||
| 0, | ||
| 50, | ||
| )}...`, | ||
| ); | ||
| } else { | ||
| throw new Error(`Certificate file not found: ${config.awmBackupServerCaCertPath}`); | ||
| } | ||
| } catch (e) { | ||
| const err = e instanceof Error ? e : new Error(String(e)); | ||
| throw new Error(`Failed to read AWM backup server CA cert: ${err.message}`); | ||
| } | ||
| } | ||
|
|
||
| if (config.awmBackupClientTlsKeyPath) { | ||
| try { | ||
| config.awmBackupClientTlsKey = fs.readFileSync(config.awmBackupClientTlsKeyPath, 'utf-8'); | ||
| logger.info( | ||
| `✓ AWM backup client key loaded from file: ${config.awmBackupClientTlsKeyPath}`, | ||
| ); | ||
| } catch (e) { | ||
| const err = e instanceof Error ? e : new Error(String(e)); | ||
| throw new Error(`Failed to read AWM backup client key: ${err.message}`); | ||
| } | ||
| } | ||
|
|
||
| if (config.awmBackupClientTlsCertPath) { | ||
| try { | ||
| config.awmBackupClientTlsCert = fs.readFileSync(config.awmBackupClientTlsCertPath, 'utf-8'); | ||
| logger.info( | ||
| `✓ AWM backup client certificate loaded from file: ${config.awmBackupClientTlsCertPath}`, | ||
| ); | ||
| } catch (e) { | ||
| const err = e instanceof Error ? e : new Error(String(e)); | ||
| throw new Error(`Failed to read AWM backup client cert: ${err.message}`); | ||
| } | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
the cert loading block for backup AWM is a near-copy of the primary cert loading above. could extract a small helper like loadCertFromPath(path, label) to avoid the duplication.
Contributor
There was a problem hiding this comment.
not impl lol. wont block on it - can do in a followup
margueriteblair
force-pushed
the
margieblair/wcn-362-akm-support-separate-awm-instances-for-user-and-backup-key
branch
3 times, most recently
from
c58c1a2 to
07d8951
Compare
pranishnepal
reviewed
May 5, 2026
| return undefined; | ||
| } | ||
| try { | ||
| const backupConfig: MasterExpressConfig = { |
Contributor
There was a problem hiding this comment.
so with this setup, it it currently possible to have fields (keys or certs) from two different instances, for example key from main client, and cert from backup - we should enforce that it's all or nothing, no? As in, either they use backup fields for all of the config or none of it. Intermixing the two will lead to random state.
Contributor
There was a problem hiding this comment.
Maybe we can add validation to the starting point of the app, the configureMasterExpressMode() maybe? For the fields that need to move together, perhaps smth like:
if (config.awmBackupClientTlsKey && !config.awmBackupClientTlsCert) {
throw new Error('backup key and cert must both exist');
}
...
Author
There was a problem hiding this comment.
Hm, yeah I'd agree with this - either provide the appropriate certs for the backup instance, or nothing
margueriteblair
force-pushed
the
margieblair/wcn-362-akm-support-separate-awm-instances-for-user-and-backup-key
branch
from
07d8951 to
233fef6
Compare
pranavjain97
reviewed
May 5, 2026
Comment on lines
+502
to
+547
| // Handle cert loading for backup AWM (only when backup URL is configured) | ||
| if (config.advancedWalletManagerBackupUrl) { | ||
| if (config.awmBackupServerCaCertPath) { | ||
| try { | ||
| if (fs.existsSync(config.awmBackupServerCaCertPath)) { | ||
| config.awmBackupServerCaCert = fs.readFileSync(config.awmBackupServerCaCertPath, 'utf-8'); | ||
| logger.info( | ||
| `✓ AWM backup server CA certificate loaded from file: ${config.awmBackupServerCaCertPath?.substring( | ||
| 0, | ||
| 50, | ||
| )}...`, | ||
| ); | ||
| } else { | ||
| throw new Error(`Certificate file not found: ${config.awmBackupServerCaCertPath}`); | ||
| } | ||
| } catch (e) { | ||
| const err = e instanceof Error ? e : new Error(String(e)); | ||
| throw new Error(`Failed to read AWM backup server CA cert: ${err.message}`); | ||
| } | ||
| } | ||
|
|
||
| if (config.awmBackupClientTlsKeyPath) { | ||
| try { | ||
| config.awmBackupClientTlsKey = fs.readFileSync(config.awmBackupClientTlsKeyPath, 'utf-8'); | ||
| logger.info( | ||
| `✓ AWM backup client key loaded from file: ${config.awmBackupClientTlsKeyPath}`, | ||
| ); | ||
| } catch (e) { | ||
| const err = e instanceof Error ? e : new Error(String(e)); | ||
| throw new Error(`Failed to read AWM backup client key: ${err.message}`); | ||
| } | ||
| } | ||
|
|
||
| if (config.awmBackupClientTlsCertPath) { | ||
| try { | ||
| config.awmBackupClientTlsCert = fs.readFileSync(config.awmBackupClientTlsCertPath, 'utf-8'); | ||
| logger.info( | ||
| `✓ AWM backup client certificate loaded from file: ${config.awmBackupClientTlsCertPath}`, | ||
| ); | ||
| } catch (e) { | ||
| const err = e instanceof Error ? e : new Error(String(e)); | ||
| throw new Error(`Failed to read AWM backup client cert: ${err.message}`); | ||
| } | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
not impl lol. wont block on it - can do in a followup
pranavjain97
approved these changes
May 5, 2026
pranishnepal
reviewed
May 5, 2026
| // Create backup client if backup URL is configured; falls back to primary client | ||
| try { | ||
| const awmBackupClient = createAwmBackupClient(bitgoReq.config, bitgoReq.params?.coin); | ||
| bitgoReq.awmBackupClient = awmBackupClient ?? awmClient; |
Contributor
There was a problem hiding this comment.
Might be worth noting this in the README that we fall back to the default
pranishnepal
approved these changes
May 5, 2026
Contributor
pranishnepal
left a comment
pranishnepal
left a comment
There was a problem hiding this comment.
LGTM, though you might need to update some tests
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ticket: WCN-362