feat(multichain-account-service): add option to decide what to await for when creating group - #6759
Conversation
…for when creating group
Co-authored-by: Charly Chevalier <charly.chevalier@consensys.net>
| // Extract the EVM provider from the list of providers. | ||
| // We will only await the EVM provider to create its accounts, while | ||
| // all other providers will be started in the background. | ||
| const [evmProvider, ...otherProviders] = this.#providers; |
There was a problem hiding this comment.
Does that assume EVM provider is always first? Can't the provider order change in theory?
There was a problem hiding this comment.
const evmProvider = this.#providers.find(p => p instanceof EvmAccountProvider);
assert(evmProvider, 'EVM account provider not found');
const otherProviders = this.#providers.filter(p => p !== evmProvider);
Maybe overkill 😋
There was a problem hiding this comment.
That's not needed IMO. We could have an extra assert after we build the list of providers to enforce this precondition (like EVM provider always comes first)
| MultichainAccountGroup<Account> | ||
| > { | ||
| return this.createMultichainAccountGroup(this.getNextGroupIndex()); | ||
| return this.createMultichainAccountGroup(this.getNextGroupIndex(), { |
There was a problem hiding this comment.
This is a breaking change isn't it? Do we have to point that out in changelog? 👀
There was a problem hiding this comment.
Last change we made around this method was also altering the behavior a bit, but we did not treat it as a breaking change either.
I think, it could be seen as such, but for now, the only guarantee that we have for the multichain account group is that we ALWAYS HAVE at least an EVM account, and that still holds true with the previous change we made AND this one IMO.
Lastly, for simplicity, I would like to avoid breaking change in those packages to avoid bubbling up to other dependents controllers (we need this fix to be merged ASAP).
If we change our mind, we'll make a new 2.0.0 and amend the changelog if really needed later!
| const { wallet, providers } = setup({ | ||
| accounts: [[mockEvmAccount]], // 1 provider | ||
| }); |
There was a problem hiding this comment.
This case only uses 1 provider but claims to test "all providers" behavior, do I read that right? Should it use multiple providers to properly test instead?
There was a problem hiding this comment.
It does claim that "if any of the provider fails", so the test is ok.
Though, I think we could have used the scenario EVM + Solana provider where Solana fails, that could have been clearer indeed 👍
| otherProviders.forEach((provider) => { | ||
| provider | ||
| .createAccounts({ |
There was a problem hiding this comment.
Shouldn't we track background promises too? Or is it the meaning to ignore failures here and future alignments will fix it? or do I miss something? 🤔
There was a problem hiding this comment.
Yes exactly, we willingly ignore failures for providers creating accounts in the background
There was a problem hiding this comment.
Yes that's expected to not throw in that case, mostly because would not be any way to catch them truly
Though, we make sure the EvmAccountProvider is successful.
If any of the other providers fail, then yes we would need an alignment for this (unfortunately, we don't have any state to tell us a group is misaligned for now, and we might need that in the near future IMO)
fabiobozzo
left a comment
There was a problem hiding this comment.
LGTM. @mathieuartu I left a couple comments as food for thoughts, no remarks
Explanation
References
Checklist
Note
Adds an
options.waitForAllProvidersToFinishCreatingAccountsflag tocreateMultichainAccountGroup, defaulting to awaiting only EVM, and updatescreateNextMultichainAccountGroupto await all.createMultichainAccountGroup:optionswithwaitForAllProvidersToFinishCreatingAccounts(defaultfalse).true: await all providers viaPromise.allSettled; throw on any failure with aggregated warnings.false(default): await EVM provider; start other providers in background, log but ignore their errors.createNextMultichainAccountGroup: now callscreateMultichainAccountGroupwithwaitForAllProvidersToFinishCreatingAccounts: true.optionsparameter and behavior.Written by Cursor Bugbot for commit 403d744. This will update automatically on new commits. Configure here.