Skip to content
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.

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

feat(sdk-coin-oas): add oas sdk skeleton #5093

Merged
merged 1 commit into from
Nov 15, 2024
Merged

Conversation

adarsh312
Copy link
Contributor

Ticket: WIN-3696

modules/bitgo/src/v2/coinFactory.ts Fixed Show fixed Hide fixed
modules/bitgo/src/v2/coinFactory.ts Fixed Show fixed Hide fixed
modules/sdk-coin-oas/test/unit/index.ts Fixed Show fixed Hide fixed
modules/sdk-coin-oas/test/unit/index.ts Fixed Show fixed Hide fixed
modules/sdk-coin-oas/test/unit/index.ts Fixed Show fixed Hide fixed
@adarsh312 adarsh312 force-pushed the WIN-3696_oas_skeleton branch from 31c091c to d0a57b1 Compare November 8, 2024 09:42
@adarsh312 adarsh312 force-pushed the WIN-3696_oas_skeleton branch 3 times, most recently from 12fcdf0 to e22f195 Compare November 11, 2024 14:02
@adarsh312 adarsh312 marked this pull request as ready for review November 12, 2024 06:23
@adarsh312 adarsh312 requested review from a team as code owners November 12, 2024 06:23
/** @inheritDoc */
allowsAccountConsolidations(): boolean {
return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not needed as it is having forwarders

Copy link
Contributor

Choose a reason for hiding this comment

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

no need to add this method only as by default it is false in the BaseCoin, can remove in a follow up

modules/sdk-coin-oas/src/oas.ts Outdated Show resolved Hide resolved
modules/sdk-coin-oas/src/oas.ts Outdated Show resolved Hide resolved
modules/sdk-coin-oas/test/integration/index.ts Outdated Show resolved Hide resolved
modules/sdk-coin-oas/test/unit/oas.ts Outdated Show resolved Hide resolved
modules/sdk-coin-oas/test/resources/oas.ts Outdated Show resolved Hide resolved
@adarsh312 adarsh312 force-pushed the WIN-3696_oas_skeleton branch 2 times, most recently from 18393d7 to 73658c0 Compare November 13, 2024 04:36
gianchandania
gianchandania previously approved these changes Nov 13, 2024
/** @inheritDoc */
allowsAccountConsolidations(): boolean {
return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to add this method only as by default it is false in the BaseCoin, can remove in a follow up

Copy link
Contributor

@zahin-mohammad zahin-mohammad left a comment

Choose a reason for hiding this comment

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

Update code owners please.

@@ -0,0 +1,106 @@
export const keyShares = {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a GG18 keyshare. Can we convert this test to use DKLS instead?

@adarsh312 adarsh312 force-pushed the WIN-3696_oas_skeleton branch 2 times, most recently from 68a3f97 to 7cb1a1d Compare November 14, 2024 09:38
export class TransactionBuilder extends AbstractTransactionBuilder {
amount(arg0: number) {
throw new Error('Method not implemented.');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this amount method seems like a mistake, please remove it

@adarsh312 adarsh312 force-pushed the WIN-3696_oas_skeleton branch from 7cb1a1d to f158abc Compare November 14, 2024 10:25
@adarsh312 adarsh312 requested a review from a team as a code owner November 14, 2024 10:25
@adarsh312 adarsh312 force-pushed the WIN-3696_oas_skeleton branch from f158abc to db07361 Compare November 14, 2024 10:57
Copy link
Contributor

@therealdwright therealdwright left a comment

Choose a reason for hiding this comment

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

CODEOWNERS LGTM.

@adarsh312 adarsh312 dismissed zahin-mohammad’s stale review November 15, 2024 05:25

required changes are completed and reviewed

@adarsh312 adarsh312 merged commit c5262dd into master Nov 15, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants