fix(wallet-key-share): Optimize createBulkWalletShare to Fetch Keychain Once and Prevent Rate Limiting

[abk404]

Ticket: CSI-1548

Type of change

• Bug fix (non-breaking change which fixes an issue)

Summary

Optimizes createBulkWalletShare to fetch the wallet keychain only once instead of once per user, drastically reducing API calls and preventing rate limiting (HTTP 429) errors during bulk wallet sharing.

Problem

When sharing a wallet with multiple users via createBulkWalletShare, the SDK was fetching the wallet keychain (v2.key.get) for each user in the share list.

For example, sharing 26 wallets with 15 users each resulted in 390 API calls (26 × 15), causing rate limiting errors and failures with the message: "Failed to process wallet shares after retrying with single requests."
Check the support ticket : CS-6701

Solution

Refactored the code to:

1. Extract common logic into two new helper methods:

   • getDecryptedKeychainForSharing() - Fetches and decrypts the keychain once
   • encryptPrvForUser() - Encrypts the private key for a specific user (no API calls)

2. Optimize createBulkWalletShare() to fetch the keychain once before the loop, then reuse it for all users

[sdoshibitgo]

Missing validation: The original code had explicit assertions for keychain fields. Consider keeping runtime validation instead of relying solely on type cast.

[abk404]

makes sense, added the explicit assertions and removed the typecast completely.

[sdoshibitgo]

Type improvement: Consider extracting { prv: string; pub: string } into a named interface for reusability.

[abk404]

Done

[sdoshibitgo]

Expand test coverage: Also verify encryptPrvForUser is called once per user to ensure encryption happens for each recipient.

[abk404]

Added

[zahin-mohammad]

Are we sure we don't already have this type defined somewhere?

[abk404]

No , we don't have this type defined somewhere else.
This was not required before because the prepareSharedKeychain was doing the operations in the same function.

Since, I broke down that function into 3 different function , we need this new interface for having the return typed for the new function.

There is a Keychain type defined which has these two fields(along with ton of other fields), but there also these fields are marked as optional .
i can do Required<Pick<Keychain, 'prv' | 'pub'>> but the current approcah of defining a new interface seems more readable and explicit.

This also follows the current pattern where a new type BulkWalletShareKeychain was created for the specific usecase instead of usign the Keychain type.

[zahin-mohammad]

Suggested change

	// Only one of pub/commonPub/commonKeychain should be present in the keychain
	let pub = keychain.pub ?? keychain.commonPub;
	if (keychain.commonKeychain) {
	// Only one of pub/commonKeychain should be present in the keychain
	let pub = keychain.pub;

commonPub is deprecated field, we shouldn't have any active wallets that are using this field.

[abk404]

The commonPub fallback was pre-existing code from the original bulk sharing implementation. This PR is a refactoring that moves this logic from prepareSharedKeychain() to getDecryptedKeychainForSharing() without changing any behaviour.

If it's a deprecated field , I can remove this, but just thinking if it will break some other flows in downstream services which calls the /key endpoint and still expect the commonPub field.

[zahin-mohammad]

Why is this necessary??

[zahin-mohammad]

Will ping on slack.

[abk404]

This was also there before in the codebase I refactored the code to pull out this pub key creation logic into a separate function.
The /walletshare/{id} responses still include the pub , so I am a little skeptical if removing the field might break some other services consuming this endpoint. Also pub is used when accepting shares.

If we want to remove pub or simplify this extraction logic, I'd suggest doing it in a separate cleanup PR to avoid scope creep and ensure proper testing.

[zahin-mohammad]

Suggested change

	const keychain: BulkWalletShareKeychain = {
	pub,
	encryptedPrv: newEncryptedPrv,
	fromPubKey: eckey.publicKey.toString('hex'),
	toPubKey: userPubkey,
	path: path,
	};
	assert(keychain.pub, 'pub must be defined for sharing');
	const keychain: BulkWalletShareKeychain = {
	encryptedPrv: newEncryptedPrv,
	fromPubKey: eckey.publicKey.toString('hex'),
	toPubKey: userPubkey,
	path: path,
	};

This shouldn't be necessary.

[abk404]

Same as above comment.