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

feat(sdk-coin-ethlike): add new ethLikeCoin package #4580

Merged
merged 1 commit into from
Jun 11, 2024
Merged

feat(sdk-coin-ethlike): add new ethLikeCoin package #4580

merged 1 commit into from
Jun 11, 2024

Conversation

mullapudipruthvik
Copy link
Contributor

Ticket: COIN-667

TICKET: COIN-667

We use the this ethLikeCoin packages to build txns for evm coins which bitgo doesn't support.
This commit also adds required changes to support
base chain coin.

@mullapudipruthvik mullapudipruthvik force-pushed the COIN-667-add-new-ethLikeCoin-package branch 9 times, most recently from 19d0e57 to 6937400 Compare June 3, 2024 05:06
mullapudipruthvik
mullapudipruthvik commented Jun 3, 2024
View reviewed changes
modules/sdk-coin-ethlike/src/ethlikeCoin.ts
return new EthLikeCoin(bitgo, staticsCoin);
}

protected getTransactionBuilder(common?: EthereumCommon): EthLikeTransactionBuilder {
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 a common param which is used to build txn for evm chains

@mullapudipruthvik mullapudipruthvik force-pushed the COIN-667-add-new-ethLikeCoin-package branch from 6937400 to f4305eb Compare June 3, 2024 05:41
@alebusse alebusse force-pushed the COIN-667-add-new-ethLikeCoin-package branch from f4305eb to 17382f7 Compare June 3, 2024 18:33
@socket-security Socket Security
Copy link

socket-security bot commented Jun 3, 2024 •
edited
Loading

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@bitgo/[email protected] None 0 2.4 MB alebusse
npm/@types/[email protected] None 0 12.3 kB types
npm/[email protected] None 0 0 B
npm/[email protected] None 0 0 B

🚮 Removed packages: npm/@babel/[email protected], npm/@jridgewell/[email protected], npm/@jridgewell/[email protected], npm/@jridgewell/[email protected], npm/@noble/[email protected], npm/@nodelib/[email protected], npm/@npmcli/[email protected], npm/@npmcli/[email protected], npm/@sinonjs/[email protected], npm/@sinonjs/[email protected], npm/@types/[email protected], npm/@types/[email protected], npm/@types/[email protected], npm/@types/[email protected], npm/@types/[email protected], npm/@types/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected]

View full report↗︎

@mullapudipruthvik mullapudipruthvik force-pushed the COIN-667-add-new-ethLikeCoin-package branch 3 times, most recently from 8cb05a3 to 8bc9a0b Compare June 3, 2024 19:07
@alebusse
Copy link
Contributor

alebusse commented Jun 3, 2024

Package updates / unit-test-all steps try to install everything without the yarn.lock and some library is getting updated to breaking change and causing issues, i will find it and fix since its not related to this PR you can ignore.

alebusse
alebusse reviewed Jun 3, 2024
View reviewed changes
modules/sdk-coin-ethlike/package.json Outdated
"@bitgo/abstract-eth": "^21.5.1",
"@bitgo/sdk-core": "^26.15.0",
"@bitgo/statics": "^48.15.0",
"@ethereumjs/common": "^2.6.5",
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
"@ethereumjs/common": "^2.6.5",
"@ethereumjs/common": "2.6.5",

Please lock the version by removing the caret symbol for external dependencies.

@mullapudipruthvik mullapudipruthvik force-pushed the COIN-667-add-new-ethLikeCoin-package branch from 8bc9a0b to eda2c8d Compare June 4, 2024 03:26
@mullapudipruthvik mullapudipruthvik marked this pull request as ready for review June 4, 2024 07:10
@mullapudipruthvik mullapudipruthvik requested review from a team as code owners June 4, 2024 07:10
ajays97
ajays97 reviewed Jun 4, 2024
View reviewed changes
modules/bitgo/src/v2/coinFactory.ts Outdated Show resolved Hide resolved
modules/bitgo/src/v2/coinFactory.ts Outdated Show resolved Hide resolved
modules/sdk-coin-ethlike/src/lib/transactionBuilder.ts
Comment on lines +10 to +14
constructor(_coinConfig: Readonly<CoinConfig>, common?: EthereumCommon) {
super(_coinConfig);
if (!common) {
throw new Error('Common must be provided for EthLikeTransactionBuilder');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we not make the common parameter an optional argument since we are throwing an error if it is not passed? Or reverse

Copy link
Contributor Author

Choose a reason for hiding this comment

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

'common` as an optional parameter and we cannot make it mandatory cause it will break other coins functionality

gianchandania
gianchandania reviewed Jun 5, 2024
View reviewed changes
Copy link
Contributor

@gianchandania gianchandania left a comment

Choose a reason for hiding this comment

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

changes lgtm, but tests are failing

@mullapudipruthvik
feat(sdk-coin-ethlike): add new eth like coin packages
ba305cb 
Ticket: COIN-667

We use the this ethLikeCoin packages to build txns for evm coins which bitgo doesn't support.
This commit also adds required changes to support
base chain coin.

TICKET: COIN-667
@mullapudipruthvik mullapudipruthvik force-pushed the COIN-667-add-new-ethLikeCoin-package branch from eda2c8d to ba305cb Compare June 10, 2024 10:25
therealdwright
therealdwright approved these changes Jun 11, 2024
View reviewed changes
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

@mullapudipruthvik mullapudipruthvik merged commit 8506362 into master Jun 11, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gianchandania gianchandania gianchandania approved these changes

@therealdwright therealdwright therealdwright approved these changes

@ajays97 ajays97 Awaiting requested review from ajays97

@alebusse alebusse Awaiting requested review from alebusse

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

Successfully merging this pull request may close these issues.
5 participants
@mullapudipruthvik @alebusse @therealdwright @ajays97 @gianchandania