refactor(wallet-sdk): remove default synchronizer auto-selection#1740
refactor(wallet-sdk): remove default synchronizer auto-selection#1740jarekr-da wants to merge 9 commits into
Conversation
jarekr-da
changed the title
jarekr-da
force-pushed
the
jarekr/sdk_synchronizers
branch
from
97545e9 to
3ae7ca0
Compare
jarekr-da
force-pushed
the
wiktor/multisync-example
branch
from
563df2d to
98dfc85
Compare
jarekr-da
force-pushed
the
jarekr/sdk_synchronizers
branch
from
3ae7ca0 to
a84acce
Compare
jarekr-da
force-pushed
the
jarekr/sdk_synchronizers
branch
from
a84acce to
fa21b86
Compare
jarekr-da
changed the title
jarekr-da
force-pushed
the
jarekr/sdk_synchronizers
branch
2 times, most recently
from
7c58d39 to
452af4a
Compare
|
Check - where syncId is really needed by canton. |
jarekr-da
force-pushed
the
jarekr/sdk_synchronizers
branch
2 times, most recently
from
0256183 to
51c5d16
Compare
jarekr-da
force-pushed
the
wiktor/multisync-example
branch
from
93653ae to
1bee65e
Compare
…vide synchronizerId Signed-off-by: jarekr-da <jaroslaw.ratajski@digitalasset.com>
…examples and snippets Signed-off-by: jarekr-da <jaroslaw.ratajski@digitalasset.com>
… for multi-sync Signed-off-by: jarekr-da <jaroslaw.ratajski@digitalasset.com>
…Id to DAR uploads in upload-dars Signed-off-by: jarekr-da <jaroslaw.ratajski@digitalasset.com>
…Id to DAR uploads and external party creation The wallet SDK no longer auto-selects a synchronizer. DAR upload and external party creation now require an explicit synchronizerId, which fails on multi-sync participants otherwise. Resolve the global synchronizer via getGlobalSynchronizerId and pass it to dar.upload and party.external.create across the affected example scripts. Signed-off-by: jarekr-da <jaroslaw.ratajski@digitalasset.com>
…arty creation in scripts 08, 09, 12 and stress 01 Signed-off-by: jarekr-da <jaroslaw.ratajski@digitalasset.com>
Signed-off-by: jarekr-da <jaroslaw.ratajski@digitalasset.com>
Signed-off-by: jarekr-da <jaroslaw.ratajski@digitalasset.com>
jarekr-da
force-pushed
the
jarekr/sdk_synchronizers
branch
from
5bf4f06 to
f937203
Compare
Signed-off-by: jarekr-da <jaroslaw.ratajski@digitalasset.com>
mateuszpiatkowski-da
left a comment
mateuszpiatkowski-da
left a comment
There was a problem hiding this comment.
the code itself looks fine, but I don't understand why can't we simply make the default synchronizer id an optional parameter when instantiating sdk. This way the user would still have the ability to point to the right resource (as described in this comment) while keeping the interface as simple as possible :)
| options?.confirmingParticipantEndpoints ?? [] | ||
| ), | ||
| options?.synchronizerId || this.resolveSynchronizerId(), | ||
| options?.synchronizerId, |
There was a problem hiding this comment.
I don't think this is needed here, as this is not async
| ]) => | ||
| this.ctx.ledgerProvider.request<Ops.PostV2PartiesExternalGenerateTopology>( | ||
| ]) => { | ||
| if (!synchronizerId) |
There was a problem hiding this comment.
you can simply pass options.synchronizerId
| this.ctx.defaultSynchronizerId | ||
| const synchronizerId = this.createPartyOptions?.synchronizerId | ||
| if (!synchronizerId) | ||
| throw new Error( |
There was a problem hiding this comment.
use sdk.ctx.error.throw
| const migrationId = params.migrationId ?? 0 | ||
| const synchronizerId = params.synchronizerId | ||
| if (!synchronizerId) | ||
| throw new Error( |
There was a problem hiding this comment.
use sdk.ctx.error.throw
| this.sdkContext.commonCtx.defaultSynchronizerId | ||
| const synchronizerId = params?.synchronizerId | ||
| if (!synchronizerId) | ||
| throw new Error( |
There was a problem hiding this comment.
use sdk.ctx.error.throw
| const provider = parties?.provider ?? this.ctx.validatorParty | ||
| const synchronizerId = | ||
| args.synchronizerId ?? this.ctx.commonCtx.defaultSynchronizerId | ||
| const synchronizerId = args.synchronizerId |
There was a problem hiding this comment.
nitpick: is this const needed? I think we can just write args.synchronizerId
|
|
||
| // ========= Resolve the synchronizer parties are hosted on ========= | ||
| const { connectedSynchronizers } = await sdk.ledger.connectedSynchronizers( | ||
| {} |
There was a problem hiding this comment.
I've commented on this in the target branch's PR - I think that the argument can be optional, as {} doesn't say much to the reader
| PATH_TO_DAR_IN_LOCALNET | ||
| ) | ||
|
|
||
| // Resolve the global synchronizer explicitly: the SDK no longer |
There was a problem hiding this comment.
nitpick: the comment refers to the prior status of SDK capabilities, but once this PR is merged this will no longer be valid as the changes will be overwritten. I suggest shortening this to:
| // Resolve the global synchronizer explicitly: the SDK no longer | |
| // Resolve the global synchronizer explicitly as the DAR upload cannot be autodetected when the | |
| // participant is connected to multiple synchronizers. |
| if (!this.sdk) throw new Error('SDK not initialized') | ||
|
|
||
| const { connectedSynchronizers } = | ||
| await this.sdk.ledger.connectedSynchronizers({}) |
| * The wallet SDK no longer auto-selects a synchronizer, so client code (these | ||
| * examples) resolves it explicitly and passes it to SDK calls that require one. | ||
| */ | ||
| export async function getGlobalSynchronizerId(sdk: { |
There was a problem hiding this comment.
One thing I don't understand - why can't wallet-sdk itself have the default behavior like described here to implement? I think it would be substantially easier for the end user to just let sdk do their own thing by default and have the possibility to optionally overwrite that behavior by passing either:
- end value
- callback incorporating appropriate business logic
- value that would be used in sdk-fixed function
Is it possible for the global synchronizer id to change during the lifecycle of the sdk instance? Can the global synchronizer id even change? Intutively this should be a constant publically accessible, so in this case synchronizerAlias should always be equal to global which in turn should simplify both the logic and the user interface of sdk
2 participants
Note:
This
complicatesall existing tests. Code must now explicitly select synchronizer in many calls.Alternative would be to start some tests in single synchronizer (but this also means, such test would fail randomly if run on multi synchronizer localnet).