fix(statics): fix unstETH support #6630
Conversation
saarujansathees578
force-pushed
the
SC-2516/fix-unsteth
branch
7 times, most recently
from
ca7e025 to
55095a1
Compare
|
Overall LGTM, just make sure it does not cause regressions |
saarujansathees578
force-pushed
the
SC-2516/fix-unsteth
branch
2 times, most recently
from
431839a to
80d7a2b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
saarujansathees578
force-pushed
the
SC-2516/fix-unsteth
branch
from
80d7a2b to
229ed36
Compare
saarujansathees578
force-pushed
the
SC-2516/fix-unsteth
branch
from
229ed36 to
0e57013
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR implements support for ERC721 NFTs on the Ethereum network, extending the existing token infrastructure to handle non-fungible tokens alongside the current ERC20 token support.
- Adds ERC721Token class with recovery functionality and NFT-specific operations
- Extends token configuration structure to include NFT support for both mainnet and testnet
- Updates coin factory registration to handle ERC721 token constructors
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/statics/src/tokenConfig.ts | Adds nfts property to token configurations and implements ERC721 token formatting |
| modules/sdk-coin-eth/src/index.ts | Exports the new ERC721Token class |
| modules/sdk-coin-eth/src/erc721Token.ts | Implements complete ERC721Token class with recovery and transaction functionality |
| modules/bitgo/test/v2/unit/ams/ams.ts | Adds comprehensive test coverage for ERC721 NFT functionality |
| modules/bitgo/test/v2/resources/amsTokenConfig.ts | Adds test configuration for ERC721 tokens |
| modules/bitgo/test/browser/browser.spec.ts | Updates browser test expectations to include ERC721Token |
| modules/bitgo/src/v2/coins/index.ts | Exports ERC721Token class |
| modules/bitgo/src/v2/coinFactory.ts | Registers ERC721 token constructors and handles token type detection |
|
|
||
| if (isKrsRecovery) { | ||
| signedTx.backupKey = backupKey; | ||
| signedTx.coin = 'erc721'; |
There was a problem hiding this comment.
The hardcoded coin value 'erc721' should use the actual coin property from tokenConfig to match the expected format (e.g., 'eth', 'hteth'). This inconsistency could cause issues in recovery operations.
| signedTx.coin = 'erc721'; | |
| signedTx.coin = this.tokenConfig.coin; |
Copilot uses AI. Check for mistakes.
| } | ||
|
|
||
| getFullName() { | ||
| return 'ERC721 Token'; |
There was a problem hiding this comment.
The hardcoded generic name 'ERC721 Token' should return the actual token name from tokenConfig.name to provide more meaningful identification, similar to how other token methods work.
| return 'ERC721 Token'; | |
| return this.tokenConfig.name; |
Copilot uses AI. Check for mistakes.
| @@ -862,7 +869,11 @@ export function getTokenConstructor(tokenConfig: TokenConfig): CoinConstructor | | |||
| switch (tokenConfig.coin) { | |||
| case 'eth': | |||
| case 'hteth': | |||
| return Erc20Token.createTokenConstructor(tokenConfig as Erc20TokenConfig); | |||
| if (tokenConfig.type.includes('erc721')) { | |||
There was a problem hiding this comment.
Using string inclusion check for token type detection is fragile. Consider using a more robust method like checking a specific token standard property or using startsWith('erc721:') to be more precise.
| if (tokenConfig.type.includes('erc721')) { | |
| if (tokenConfig.type.startsWith('erc721')) { |
Copilot uses AI. Check for mistakes.
TICKET: SC-2516