refactor: adding eth & tokenfactory types #389
Conversation
WalkthroughThe pull request introduces modifications across three files related to Ethereum and token factory message handling. New constants and types are added to Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning Rate limit exceeded@CalicoNino has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 3 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
src/sdk/msg/eth.ts (1)
16-21: Consider enhancing type safety for message type URLsWhile the implementation is correct, we can improve type safety by using a const assertion and creating a type from the constant.
Consider this enhancement:
-export const ETH_MSG_TYPE_URLS = { +export const ETH_MSG_TYPE_URLS = { MsgEthereumTx: `/${MsgServiceName}EthereumTx`, MsgUpdateParams: `/${MsgServiceName}UpdateParams`, MsgCreateFunToken: `/${MsgServiceName}CreateFunToken`, MsgConvertCoinToEvm: `/${MsgServiceName}ConvertCoinToEvm`, -} +} as const + +export type EthMsgTypeUrl = typeof ETH_MSG_TYPE_URLS[keyof typeof ETH_MSG_TYPE_URLS]src/sdk/tx/txClient.ts (2)
16-21: Consider using explicit import paths.While the relative import
".."works, it's more maintainable to use explicit paths. This helps prevent issues if the directory structure changes and makes the code dependencies clearer.-} from ".." +} from "../msg/eth" +} from "../msg/tokenfactory" +} from "../core/extensions"
29-31: LGTM! Consider documenting type resolution order.The addition of Ethereum and Tokenfactory types is implemented correctly. Since type resolution can depend on the order of types in the registry, consider adding a comment explaining why this specific order was chosen.
export const nibiruRegistryTypes: ReadonlyArray<[string, GeneratedType]> = [ + // Base types first, followed by application-specific types ...defaultRegistryTypes, ...tokenfactoryTypes, ...ethTypes, ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/sdk/msg/eth.ts(2 hunks)src/sdk/msg/tokenfactory.ts(3 hunks)src/sdk/tx/txClient.ts(3 hunks)
🔇 Additional comments (6)
src/sdk/msg/eth.ts (2)
Line range hint 3-14: LGTM: Well-structured imports
The imports are properly organized and include all necessary types for Ethereum message handling.
23-28: Verify message type consistency across the codebase
The type registry looks correct, but let's ensure all message types are consistently used across the codebase.
✅ Verification successful
Message type registry is correctly implemented
The verification shows that the message types in ethTypes are properly registered and used consistently across the codebase:
MsgEthereumTxis used in eth query responsesMsgUpdateParamsappears in devgas module with proper imports and handlersMsgCreateFunTokenandMsgConvertCoinToEvmare new message types being introduced- No inconsistent or conflicting usage of these message types was found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify message type consistency and usage
# Check for any message type references that might be missing from the registry
rg -t typescript "Msg(EthereumTx|UpdateParams|CreateFunToken|ConvertCoinToEvm)" \
--glob '!src/sdk/msg/eth.ts' \
--glob '!**/*.test.ts' \
-A 2
# Check for any potential message handlers that might need updating
ast-grep --pattern 'interface $_ {
$$$
ethereum$_($$$): $_
$$$
}'
Length of output: 247
Script:
#!/bin/bash
# Search for message type references using ripgrep without file type restriction
rg "Msg(EthereumTx|UpdateParams|CreateFunToken|ConvertCoinToEvm)" \
--glob '!src/sdk/msg/eth.ts' \
--glob '!**/*.test.ts' \
-A 2
# Search for ETH_MSG_TYPE_URLS usage
rg "ETH_MSG_TYPE_URLS\." \
--glob '!src/sdk/msg/eth.ts' \
-A 1
# Check for any ethereum-related message handlers
rg "ethereum.*Message" \
--glob '!src/sdk/msg/eth.ts' \
-A 2
Length of output: 1229
src/sdk/tx/txClient.ts (1)
70-70: Verify transaction type handling across clients.
The addition of the registry configuration to the wasmClient ensures consistent type handling. Let's verify that all transaction types are properly registered and can be handled.
✅ Verification successful
Transaction type registry configuration is properly implemented
The verification shows that:
- The
nibiruRegistryTypescorrectly includes all necessary message types by combining:- Default registry types
- Tokenfactory message types (CreateDenom, ChangeAdmin, Mint, Burn, etc.)
- Ethereum message types (EthereumTx, UpdateParams, CreateFunToken, etc.)
- The Registry is consistently initialized with these types in both the regular client and wasmClient configurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all required message types are included in the registry
# Check for message type definitions in eth.ts
echo "Checking Ethereum message types:"
ast-grep --pattern 'export const ethTypes: ReadonlyArray<[string, GeneratedType]> = [
$$$
]'
# Check for message type definitions in tokenfactory.ts
echo "Checking Tokenfactory message types:"
ast-grep --pattern 'export const tokenfactoryTypes: ReadonlyArray<[string, GeneratedType]> = [
$$$
]'
# Check for any potential message types that might be missing from the registry
echo "Checking for potential missing message types:"
rg -t ts "type Msg\w+" --glob "src/sdk/msg/**/*.ts"
Length of output: 1739
Script:
#!/bin/bash
# Let's verify the registry initialization and usage
# First, check how nibiruRegistryTypes is constructed
echo "Checking nibiruRegistryTypes construction:"
rg -A 10 "nibiruRegistryTypes.*="
# Check if there are any other message type arrays that might need to be included
echo "Checking for other message type arrays:"
rg -t ts "export const \w+Types: ReadonlyArray<\[string, GeneratedType\]> = \["
# Check the Registry usage to ensure proper type registration
echo "Checking Registry usage:"
ast-grep --pattern 'new Registry($_)'
Length of output: 1646
src/sdk/msg/tokenfactory.ts (3)
Line range hint 3-20: LGTM! Imports are well-organized and properly utilized.
All imported types are being used appropriately throughout the file.
83-99: LGTM! Exports are complete and well-organized.
All necessary types and their corresponding response types are properly exported.
32-40: Verify integration with transaction client
Let's ensure that these tokenfactory types are properly integrated with the transaction client.
✅ Verification successful
Tokenfactory types are properly integrated with transaction client
The verification shows that:
- Tokenfactory types are correctly imported and included in the
nibiruRegistryTypes - The types are properly registered in both the main client and wasm client through the Registry
- Message type URLs are well-defined and match the registered types
- The TokenFactoryMsgExtension interface provides the necessary message handlers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if tokenfactoryTypes is properly imported and used in the transaction client
# Look for tokenfactoryTypes usage in txClient
echo "Checking tokenfactoryTypes integration:"
rg -A 5 "tokenfactoryTypes" src/sdk/tx/
# Verify if all message types are handled
echo "Verifying message type handling:"
rg -A 5 "MsgBurn|MsgMint|MsgCreateDenom|MsgChangeAdmin|MsgUpdateModuleParams|MsgSetDenomMetadata|MsgBurnNative" src/sdk/tx/
Length of output: 916
Script:
#!/bin/bash
# Let's verify the integration more thoroughly by:
# 1. Checking the complete registry setup
# 2. Looking for any message handlers or methods using these types
# Check the complete registry setup context
rg -B 5 -A 10 "registry\.register" src/sdk/tx/
# Look for any methods or handlers related to tokenfactory
ast-grep --pattern 'async $FUNC($$$) {
$$$
tokenfactory
$$$
}'
# Check if there are any specific tokenfactory transaction methods
rg -B 2 -A 5 "tokenfactory" src/sdk/tx/
Length of output: 986
Script:
#!/bin/bash
# Let's verify the complete message type registration setup
# 1. Check the imports and type definitions
# 2. Verify the message type URLs match the registered types
# Check the tokenfactory message type URLs definition
rg -A 10 "TOKENFACTORY_MSG_TYPE_URLS" src/
# Check the complete message type registration context
rg -B 10 -A 10 "nibiruRegistryTypes" src/
# Look for any potential message handlers in the client
ast-grep --pattern 'class NibiruTxClient {
$$$
}'
Length of output: 4893
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/sdk/msg/tokenfactory.ts (2)
32-40: Consider adding JSDoc documentation for better maintainability.The registry types array is well-structured and type-safe. Consider adding JSDoc documentation to explain the purpose and usage of this constant for future maintainers.
Add documentation like this:
+/** + * Registry of token factory message types and their corresponding protobuf message classes. + * Used for message encoding/decoding in transaction handling. + */ export const tokenfactoryTypes: ReadonlyArray<[string, GeneratedType]> = [
83-99: Consider grouping related exports for better readability.The exports are comprehensive and make all necessary types available. Consider grouping related message types with their responses using object destructuring for better organization.
Reorganize exports like this:
-export { - MsgCreateDenom, - MsgClientImpl, - MsgCreateDenomResponse, - MsgChangeAdmin, - MsgChangeAdminResponse, - MsgUpdateModuleParams, - MsgUpdateModuleParamsResponse, - MsgBurn, - MsgBurnResponse, - MsgMint, - MsgMintResponse, - MsgSetDenomMetadata, - MsgSetDenomMetadataResponse, - MsgBurnNative, - MsgBurnNativeResponse, -} +export { + // Client implementation + MsgClientImpl, + + // CreateDenom messages + MsgCreateDenom, + MsgCreateDenomResponse, + + // Admin messages + MsgChangeAdmin, + MsgChangeAdminResponse, + MsgUpdateModuleParams, + MsgUpdateModuleParamsResponse, + + // Token operations + MsgMint, + MsgMintResponse, + MsgBurn, + MsgBurnResponse, + MsgBurnNative, + MsgBurnNativeResponse, + + // Metadata operations + MsgSetDenomMetadata, + MsgSetDenomMetadataResponse, +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/sdk/msg/tokenfactory.ts(3 hunks)
🔇 Additional comments (2)
src/sdk/msg/tokenfactory.ts (2)
Line range hint 3-20: LGTM! Imports are well-organized and all are utilized.
The imports are properly structured and all imported types are used within the file.
22-30: LGTM! Message type URLs are well-defined and consistent.
The constant properly maps all message types to their corresponding URLs using a consistent pattern with MsgServiceName.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/sdk/tx/txClient.ts (2)
16-21: Consider using explicit import paths instead of barrel imports.The relative import from ".." can make the code harder to maintain and potentially impact build times. Consider using explicit import paths to clearly show where these types come from.
-import { - ethTypes, - NibiruExtensions, - setupNibiruExtension, - tokenfactoryTypes, -} from ".." +import { ethTypes } from "../msg/eth" +import { tokenfactoryTypes } from "../msg/tokenfactory" +import { NibiruExtensions, setupNibiruExtension } from "../core/extensions"
Line range hint
67-77: Consider making gas price configurable.The gas price is hardcoded to "0.025unibi" in both the wasmClient and main client options. Consider making this configurable through environment variables or constructor options to support different environments (testnet, mainnet, etc.).
+const DEFAULT_GAS_PRICE = "0.025unibi" + public static async connectWithSigner( endpoint: string, signer: OfflineSigner, options: SigningStargateClientOptions = {}, wasmOptions: NibiSigningCosmWasmClientOptions = {} ): Promise<NibiruTxClient> { const tmClient = await Tendermint37Client.connect(endpoint) + const gasPrice = process.env.NIBI_GAS_PRICE || DEFAULT_GAS_PRICE const wasmClient = await NibiSigningCosmWasmClient.connectWithSigner( endpoint, signer, { - gasPrice: GasPrice.fromString("0.025unibi"), + gasPrice: GasPrice.fromString(gasPrice), accountParser: accountFromNibiru, ...wasmOptions, } )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/sdk/tx/txClient.ts(2 hunks)
🔇 Additional comments (2)
src/sdk/tx/txClient.ts (2)
Line range hint 1-100: Overall implementation looks good!
The changes successfully integrate Ethereum and Tokenfactory types into the transaction client while maintaining backward compatibility. The implementation aligns well with the PR objectives of supporting Ethereum Funtokens and Tokenfactory transactions.
29-31: Verify potential type conflicts in registry.
The addition of new types looks good, but we should verify there are no conflicting message type URLs between the default registry and the new types.
✅ Verification successful
No type URL conflicts found in registry types
After examining the type definitions, there are no conflicts between the message type URLs:
ethTypesuses URLs prefixed with/eth/evm/v1/txtokenfactoryTypesuses URLs prefixed with/nibiru/tokenfactory/v1/tx- These are distinct from the default registry types
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for duplicate message type URLs across registry types
# Search for message type URL definitions
rg -A 1 'typeUrl.*=' src/sdk/msg/
Length of output: 35
Script:
#!/bin/bash
# Let's first locate the registry type files
fd "types?.ts" src/sdk
# Then examine their content
echo "=== Content of found type files ==="
fd "types?.ts" src/sdk --exec cat {}
Length of output: 1299
Script:
#!/bin/bash
# Let's search for the registry type definitions and imports
echo "=== Searching for registry types ==="
rg -l "defaultRegistryTypes|tokenfactoryTypes|ethTypes" src/
echo "=== Content of files containing registry types ==="
rg -l "defaultRegistryTypes|tokenfactoryTypes|ethTypes" src/ --exec cat {}
Length of output: 873
Script:
#!/bin/bash
# Let's examine the content of the relevant files individually
echo "=== Content of txClient.ts ==="
cat src/sdk/tx/txClient.ts
echo -e "\n=== Content of eth.ts ==="
cat src/sdk/msg/eth.ts
echo -e "\n=== Content of tokenfactory.ts ==="
cat src/sdk/msg/tokenfactory.ts
Length of output: 8444
|
* chore: update eth proto to latest * refactor: updating to latest graphql schema since featureflags don't include spot & perp (#385) * fix: test * fix: test * chore: upgrade graphql (#387) * refactor: adding eth & tokenfactory types (#389) * refactor: adding eth & tokenfactory types * chore: remove test file * fix: typo * refactor: wasmclient doesn't need nibi registry --------- Co-authored-by: Calico Nino <[email protected]>
## [6.0.0](v5.0.3...v6.0.0) (2024-12-02) ### Bug Fixes * test ([9d7ff5c](9d7ff5c)) * test ([e0b49c9](e0b49c9)) ### Miscellaneous Chores * update eth proto to latest ([96014f2](96014f2)) * upgrade graphql ([#387](#387)) ([d707e12](d707e12)) ### Code Refactors * adding eth & tokenfactory types ([#389](#389)) ([8c6c34c](8c6c34c)) * updating to latest graphql schema since featureflags don't include spot & perp ([#385](#385)) ([cbb36d7](cbb36d7)) [skip ci]
|
🎉 This PR is included in version 6.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
In order to signAndBroadcat Eth Funtokens and Tokenfactory, we need to add them to the registry types.
This can be tested locally via the following script
Summary by CodeRabbit
New Features
Bug Fixes
Documentation