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.

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

add abstract-eth classes #3978

Merged
merged 1 commit into from
Oct 13, 2023
Merged

add abstract-eth classes #3978

merged 1 commit into from
Oct 13, 2023

Conversation

gianchandania
Copy link
Contributor

@gianchandania gianchandania commented Oct 12, 2023 •
edited
Loading

This PR adds AbstractEthLikeMPCCoin and EthLikeMPCToken classes which will have all the common methods used by Ethlike MPC coins

@gianchandania gianchandania force-pushed the WIN-497-add-ethliketsscoin-class branch 2 times, most recently from 8808fee to 83ea644 Compare October 12, 2023 15:38
@gianchandania gianchandania changed the title Win 497 add ethliketsscoin class add abstractEthLikeTssCoin class Oct 12, 2023
@gianchandania gianchandania marked this pull request as ready for review October 12, 2023 15:43
@gianchandania gianchandania requested review from a team as code owners October 12, 2023 15:43
sachushaji
sachushaji reviewed Oct 12, 2023
View reviewed changes
modules/abstract-eth/src/ethLikeToken.ts Outdated
import { TransactionBuilder as EthTransactionBuilder, TransactionPrebuild } from '@bitgo/sdk-coin-eth';

export type CoinNames = {
[network: string]: string;
};

export class EthLikeToken extends AbstractEthLikeCoin {
export class EthLikeToken extends AbstractEthLikeTssCoin {
Copy link
Contributor

Choose a reason for hiding this comment

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

We would need an EthLikeTokenTss? will this mean all the token implementation even for multisig will have tss enabled?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on above comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added EthLikeMPCToken class

mohammadalfaiyazbitgo
mohammadalfaiyazbitgo reviewed Oct 12, 2023
View reviewed changes
Copy link
Contributor

@mohammadalfaiyazbitgo mohammadalfaiyazbitgo left a comment

Choose a reason for hiding this comment

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

Using ECDSA in the class name is more descriptive than using EthLike imo, and do these changes need to be extended to polygon as well?

modules/abstract-eth/src/abstractEthLikeTssCoin.ts Outdated
import { MPCAlgorithm } from '@bitgo/sdk-core';
import { AbstractEthLikeCoin } from './abstractEthLikeCoin';

export abstract class AbstractEthLikeTssCoin extends AbstractEthLikeCoin {
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
export abstract class AbstractEthLikeTssCoin extends AbstractEthLikeCoin {
export abstract class AbstractEcdsaCoin extends AbstractEthLikeCoin {

modules/abstract-eth/src/ethLikeToken.ts Outdated
import { TransactionBuilder as EthTransactionBuilder, TransactionPrebuild } from '@bitgo/sdk-coin-eth';

export type CoinNames = {
[network: string]: string;
};

export class EthLikeToken extends AbstractEthLikeCoin {
export class EthLikeToken extends AbstractEthLikeTssCoin {
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on above comment

@gianchandania
Copy link
Contributor Author

gianchandania commented Oct 13, 2023 •
edited
Loading

Using ECDSA in the class name is more descriptive than using EthLike imo, and do these changes need to be extended to polygon as well?

Actually this class will be specific to EthLike coins i.e. EVM compatible coins only like arbitrum, optimism, etc. Using ECDSA in the name will give an impression that it is generic because there can be other ecdsa coins as well like cosmos, hence the name. We are mainly adding this class so all the common methods which are required for the upcoming EVM compatible chains will reside here. Polygon supports both multisig and TSS wallets, so the Polygon class has many methods which are specific to multisig implementation and it is currently extending Eth class. So we won't be able to extend this to Polygon.

@gianchandania gianchandania force-pushed the WIN-497-add-ethliketsscoin-class branch from b27345b to eb99545 Compare October 13, 2023 11:02
@gianchandania gianchandania changed the title add abstractEthLikeTssCoin class add abstractEthLike classes Oct 13, 2023
@gianchandania gianchandania changed the title add abstractEthLike classes add abstract-eth classes Oct 13, 2023
@gianchandania gianchandania merged commit 29cc413 into master Oct 13, 2023
4 checks passed
@sachushaji sachushaji deleted the WIN-497-add-ethliketsscoin-class branch October 13, 2023 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sachushaji sachushaji sachushaji approved these changes

@mohammadalfaiyazbitgo mohammadalfaiyazbitgo Awaiting requested review from mohammadalfaiyazbitgo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gianchandania @sachushaji @mohammadalfaiyazbitgo