Conversation
|
@claude please review |
abhijit0943
left a comment
There was a problem hiding this comment.
Review
Good work overall - coin class, registration, network config, and tests all look solid. A few things need to be addressed before merge:
1. Missing devDependencies (will break CI)
test/unit/irys.ts imports @bitgo/sdk-test and @bitgo/sdk-api, but neither is in devDependencies. These were removed from package.json in a recent cleanup on SC-5391. Please add them back:
"devDependencies": {
"@bitgo/sdk-api": "^1.73.4",
"@bitgo/sdk-test": "^9.1.25",
...existing devDeps
}2. Missing tsconfig references
The references array in tsconfig.json was emptied on SC-5391 since there were no imports needing them at the time. Your new code imports from @bitgo/abstract-eth, @bitgo/sdk-core, and @bitgo/statics, so references need to be restored:
"references": [
{ "path": "../abstract-eth" },
{ "path": "../sdk-core" },
{ "path": "../statics" }
]3. Merge conflict on package.json
The last commit on SC-5391 removed @bitgo/abstract-eth, @bitgo/sdk-core, @bitgo/statics from dependencies (they weren't used at the time). Your PR adds them back along with @ethereumjs/common. You'll need to rebase on the latest SC-5391 and resolve the conflict.
4. getContractData stub in transactionBuilder.ts
protected getContractData(addresses: string[]): string {
throw new Error('Method not implemented.');
}This will throw at runtime if any inherited EVM code path calls it. Please add a comment explaining why it's intentionally unimplemented (Irys staking uses commitment transactions, not contract calls), so future readers don't assume it's a TODO.
5. coinFactory.ts registration (Task A9)
The plan calls for manual registration in modules/bitgo/src/v2/coinFactory.ts so that bitgo.coin('irys') works in production. Since SHARED_EVM_SDK was removed in PR #8114, irys won't be auto-registered. This needs to be included in this PR:
// In modules/bitgo/src/v2/coinFactory.ts
import { Irys, TIrys } from '@bitgo/sdk-coin-irys';
coinFactory.register('irys', Irys.createInstance);
coinFactory.register('tirys', TIrys.createInstance);And @bitgo/sdk-coin-irys needs to be added as a dependency in modules/bitgo/package.json.
|
@abhijit0943 Have addressed the comments |
abhijit0943
left a comment
There was a problem hiding this comment.
All 5 items from the previous review are addressed - looks good.
One minor nit: in transactionBuilder.ts, getCommon as getAbstractCommon is imported from @bitgo/abstract-eth but never used (the file defines its own local getCommon function). This unused import may fail the lint check - just remove it.
Ticket: SC-5394