Add EphemeralBaseAccountProvider for isolated payment flows #188
Conversation
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
Collaborator
🟡 Heimdall Review Status
|
…o enable it Separates telemetry initialization from global initialization so that: - First SDK call with telemetry: false doesn't prevent later calls from enabling telemetry - Telemetry is only loaded when at least one SDK instance requests it
Collaborator
fan-zhang-sv
left a comment
fan-zhang-sv
left a comment
There was a problem hiding this comment.
hmmm i think there might still be some issues with this approach.
i wonder if we could
- switch to dependency injection, so we inject
storeupon creating sdk instance - and have ephemeral sdk use its own store instance upon creation
- and have ephemeral sdk removing this instance store upon clear()
- this way, there might be a chance we dont need to duplicate Provider/Signer code (not 100% sure)
|
|
||
| async handshake(args: RequestArguments) { | ||
| const correlationId = correlationIds.get(args); | ||
| logHandshakeStarted({ method: args.method, correlationId }); |
Collaborator
There was a problem hiding this comment.
can we add an additional param isEphemeral to measure success for each component?
| */ | ||
| export class EphemeralSigner { | ||
| private readonly communicator: Communicator; | ||
| private readonly keyManager: SCWKeyManager; |
Collaborator
There was a problem hiding this comment.
hmm i think keyManager could still be an issue
- keyManager is still reading and writing to global state in localStorage
- keyManager.clear() rotate the global key pair, which may intervene with concurrent operation
Collaborator
Author
There was a problem hiding this comment.
Good call. Added createStoreInstance() that can create either persistent (localStorage) or ephemeral (in-memory) store instances.
Now Signer/EphemeralSigner: Both accept a storeInstance parameter and pass it down to KeyManager
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What changed? Why?
Adds
EphemeralBaseAccountProviderandEphemeralSignerfor single-use payment flows (pay()andsubscribe()).Problem: When multiple payment flows execute concurrently or sequentially, they share global state which causes race conditions and cleanup of one flow affects others.
Solution:
EphemeralBaseAccountProvider: Provider with isolated state that only supports payment methods (wallet_sendCalls,wallet_sign,wallet_getCallsStatus)EphemeralSigner: Signer that maintains instance-local state and cleans up without affecting global storesdkManager.tsto prevent concurrent payment operations from interferingcreateBaseAccountSDK.tsto avoid redundant setup callsSigner.cleanup()to preservestore.chainssince chain clients are shared infrastructureHow was this tested?
Existing test coverage for
createBaseAccountSDK.test.tsupdated to reset global initialization state between tests.How can reviewers manually test these changes?
pay()orsubscribe()multiple times in sequenceDemo/screenshots