Skip to content

chore: parallellize keychain and queryPromise #6136

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

chore: parallellize keychain and queryPromise #6136

wants to merge 1 commit into from
+26 −7

Conversation

Gautam2305
Copy link

@Gautam2305 Gautam2305 commented May 16, 2025 •
edited by jira bot
Loading

Ticket: CAAS-7

  • parallellize keychain and queryPromise in prebuildAndSignTransaction function to improve /sendcoins endpoint performance

@Gautam2305 Gautam2305 force-pushed the CAAS-7-round-trips branch 2 times, most recently from e0befb2 to 7baae82 Compare May 16, 2025 11:06
ppongbitgo
ppongbitgo reviewed May 16, 2025
View reviewed changes
@Gautam2305 Gautam2305 force-pushed the CAAS-7-round-trips branch 2 times, most recently from 8ecb7ea to bbb3faa Compare May 20, 2025 07:40
github-advanced-security[bot]
github-advanced-security bot found potential problems May 20, 2025
View reviewed changes
@Gautam2305 Gautam2305 force-pushed the CAAS-7-round-trips branch from c682e7c to 2ba5ea3 Compare May 20, 2025 09:00
github-advanced-security[bot]
github-advanced-security bot found potential problems May 20, 2025
View reviewed changes
@Gautam2305 Gautam2305 force-pushed the CAAS-7-round-trips branch from 87c70b9 to c829563 Compare May 20, 2025 10:02
@Gautam2305 Gautam2305 marked this pull request as ready for review May 20, 2025 10:34
@Gautam2305 Gautam2305 requested review from a team as code owners May 20, 2025 10:34
@Gautam2305 Gautam2305 force-pushed the CAAS-7-round-trips branch from 4bd8ccb to 037ee71 Compare May 20, 2025 11:48
shashankms288
shashankms288 previously approved these changes May 20, 2025
View reviewed changes
Copy link
Contributor

@shashankms288 shashankms288 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

ppongbitgo
ppongbitgo reviewed May 20, 2025
View reviewed changes
modules/bitgo/test/v2/unit/wallet.ts
}
try {
await ethWallet.sendMany({ ...sendManyParamsCorrectPassPhrase });
} catch (e) {
e.message.should.not.equal(errorMessage);
e.message.should.equal(errorMessage);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did we change this test?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After making validation and prebuildQuery parallel, it would give a consistent error message unable to decrypt keychain with the given wallet passphrase. This test explicitly checked for error message to be different

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the error message changes it might break client's code - I don't think we should merge this until we can fix this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please revert tests change

@Gautam2305 Gautam2305 dismissed stale reviews from shashankms288 and margueriteblair via cd404c2 May 23, 2025 10:50
@Gautam2305 Gautam2305 force-pushed the CAAS-7-round-trips branch 2 times, most recently from cd6d3e3 to a8c3591 Compare May 23, 2025 11:35
zahin-mohammad
zahin-mohammad requested changes May 26, 2025
View reviewed changes
modules/sdk-core/src/bitgo/wallet/wallet.ts Outdated
Comment on lines 2198 to 2201
[keychains, txPrebuild] = (await Promise.all([keychainPromise, txPrebuildQuery])) as [
Keychain[],
PrebuildTransactionResult
];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[keychains, txPrebuild] = (await Promise.all([keychainPromise, txPrebuildQuery])) as [
Keychain[],
PrebuildTransactionResult
];
[keychains, txPrebuild] = (await Promise.allSettled([keychainPromise, txPrebuildQuery])) as [
Keychain[],
PrebuildTransactionResult
];

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is more suitable. With this API you can handle errors for a specific promise instead of guessing based on your caught error.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure will update it now thanks

@Gautam2305 Gautam2305 force-pushed the CAAS-7-round-trips branch from d23b489 to 6a78c59 Compare May 27, 2025 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zahin-mohammad zahin-mohammad zahin-mohammad requested changes

@shashankms288 shashankms288 shashankms288 left review comments

@lokesh-bitgo lokesh-bitgo Awaiting requested review from lokesh-bitgo lokesh-bitgo is a code owner automatically assigned from BitGo/wallet-platform

@sasikumar-bitgo sasikumar-bitgo Awaiting requested review from sasikumar-bitgo sasikumar-bitgo is a code owner automatically assigned from BitGo/wallet-platform

@ppongbitgo ppongbitgo Awaiting requested review from ppongbitgo

@margueriteblair margueriteblair Awaiting requested review from margueriteblair

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
DO NOT MERGE
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Gautam2305 @zahin-mohammad @margueriteblair @ppongbitgo @shashankms288