BE-594/domain/cosmos | cosmos/auth, cosmos/tx packages #526
Conversation
Adds cosmos/auth, cosmos/tx packages containing various high level helper functions and wrappers abstracting lower level details interacting with the chain.
WalkthroughThis pull request introduces a Changes
Possibly related PRs
Suggested reviewers
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (10)
domain/mocks/auth.go (2)
11-13: Well-structured mock with room for improved documentation.The
AuthQueryClientMockstruct is well-designed for flexible mocking. TheGetAccountFuncfield allows for easy customization of the mock's behavior in tests.Consider adding a comment to the struct and its field to improve documentation:
+// AuthQueryClientMock mocks the QueryClient interface for testing purposes. type AuthQueryClientMock struct { + // GetAccountFunc mocks the GetAccount method. If set, it will be called instead of the default implementation. GetAccountFunc func(ctx context.Context, address string) (*authtypes.QueryAccountResponse, error) }
15-20: Solid mock implementation with room for enhancement.The
GetAccountmethod implementation is correct and follows good mocking practices. It delegates to the mock function when set and panics with a clear message when not set.Consider enhancing the mock to return a default error instead of panicking when
GetAccountFuncis not set. This can make tests more robust and easier to debug:func (m *AuthQueryClientMock) GetAccount(ctx context.Context, address string) (*authtypes.QueryAccountResponse, error) { if m.GetAccountFunc != nil { return m.GetAccountFunc(ctx, address) } - panic("GetAccountFunc has not been mocked") + return nil, fmt.Errorf("GetAccountFunc has not been mocked") }Don't forget to add the
fmtimport if you make this change.domain/mocks/keyring.go (1)
15-19: LGTM: Well-designed mock struct with a minor suggestion.The
Keyringstruct is well-designed for flexible mocking of thekeyring.Keyringinterface. Each function field corresponds to a method in the interface, allowing easy customization of mock behavior in tests.Consider adding comments to each field explaining their purpose and expected return types. This would improve the readability and maintainability of the code. For example:
type Keyring struct { // GetKeyFunc returns a mock secp256k1 private key GetKeyFunc func() secp256k1.PrivKey // GetAddressFunc returns a mock account address GetAddressFunc func() sdk.AccAddress // GetPubKeyFunc returns a mock public key GetPubKeyFunc func() cryptotypes.PubKey }domain/mocks/txfees_client.go (3)
13-20: LGTM: TxFeesQueryClient struct is well-designed for mocking.The
TxFeesQueryClientstruct is well-designed with function fields corresponding to each method in thetxfeestypes.QueryClientinterface. This approach allows for flexible mocking of individual methods.Consider adding a comment above the struct to briefly explain its purpose and usage in tests. For example:
// TxFeesQueryClient is a mock implementation of txfeestypes.QueryClient. // It allows for custom implementations of each method to be provided for testing. type TxFeesQueryClient struct { // ... existing fields ... }
22-34: LGTM: FeeTokens and DenomSpotPrice methods are correctly implemented.Both methods follow a consistent and appropriate pattern for mock implementations. They correctly use the function fields and pass through all parameters.
Consider using a helper function to reduce code duplication across all methods. For example:
func callOrPanic(f interface{}, args ...interface{}) []interface{} { if f == nil { panic("unimplemented") } return reflect.ValueOf(f).Call(reflectArgs(args)) } func (m *TxFeesQueryClient) FeeTokens(ctx context.Context, in *txfeestypes.QueryFeeTokensRequest, opts ...grpc.CallOption) (*txfeestypes.QueryFeeTokensResponse, error) { res := callOrPanic(m.FeeTokensFunc, ctx, in, opts) return res[0].(*txfeestypes.QueryFeeTokensResponse), res[1].(error) }This would make the code more DRY and easier to maintain.
50-55: LGTM: GetEipBaseFee method is correctly implemented.The GetEipBaseFee method maintains the consistent implementation pattern seen in the other methods. It correctly uses the function field and passes through all parameters.
To improve the overall quality and maintainability of the mock, consider the following suggestions:
- Implement the helper function as suggested in previous comments to reduce code duplication.
- Add error handling to prevent panics in production code. For example:
func (m *TxFeesQueryClient) GetEipBaseFee(ctx context.Context, in *txfeestypes.QueryEipBaseFeeRequest, opts ...grpc.CallOption) (*txfeestypes.QueryEipBaseFeeResponse, error) { if m.GetEipBaseFeeFunc == nil { return nil, errors.New("GetEipBaseFee: unimplemented") } return m.GetEipBaseFeeFunc(ctx, in, opts...) }
- Consider adding a
NewTxFeesQueryClient()function that returns aTxFeesQueryClientwith default (no-op) implementations for all methods. This would make it easier to use the mock in tests where only specific methods need to be overridden.These changes would make the mock more robust and easier to use in various testing scenarios.
domain/mocks/txservice_client.go (1)
13-23: Well-structured mock client with room for minor improvement.The
ServiceClientstruct is well-designed, with function fields corresponding to each method in thetxtypes.ServiceClientinterface. This approach allows for flexible mocking of individual methods, which is excellent for testing various scenarios.To further enhance the code:
Consider adding a comment above the struct definition to briefly explain its purpose and usage. For example:
// ServiceClient is a mock implementation of txtypes.ServiceClient. // It allows for custom implementations of each method to be set for testing purposes. type ServiceClient struct { // ... (existing fields) }This addition would provide quick context for developers using this mock in their tests.
domain/cosmos/auth/types/types.go (1)
1-77: Add unit tests for theGetAccountmethodTo ensure the correctness and reliability of the
GetAccountmethod, consider adding unit tests that cover both successful responses and error scenarios.🧰 Tools
🪛 GitHub Check: test_prices
[failure] 43-43:
undefined: http.Get🪛 GitHub Check: test_routes
[failure] 43-43:
undefined: http.Get🪛 GitHub Check: Run linter
[failure] 1-1:
: # github.com/osmosis-labs/sqs/domain/cosmos/auth/types
[failure] 43-43:
undefined: http.Get (typecheck)
[failure] 43-43:
undefined: http.Get) (typecheck)🪛 GitHub Check: test_coingecko_prices
[failure] 43-43:
undefined: http.Get🪛 GitHub Check: test
[failure] 43-43:
undefined: http.Getdomain/cosmos/tx/tx.go (2)
121-121: Remove unnecessary// nolintdirectiveAt line 121:
// nolintThis
nolintcomment is not suppressing any specific linter warnings and may be unnecessary.Consider removing it or specifying the linter directive if it serves a particular purpose.
144-145: Reevaluate the gas adjustment factorIn
SimulateMsgs, you set the gas adjustment factor to1.05:txFactory = txFactory.WithGasAdjustment(1.05)A 5% adjustment might be insufficient for fluctuating gas consumption. Consider increasing the adjustment factor to provide a more robust buffer.
For example:
-txFactory = txFactory.WithGasAdjustment(1.05) +txFactory = txFactory.WithGasAdjustment(1.2)This sets a 20% gas adjustment to accommodate variations.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (3)
domain/cosmos/auth/types/types_test.gois excluded by!**/*_test.godomain/cosmos/tx/export_test.gois excluded by!**/*_test.godomain/cosmos/tx/tx_test.gois excluded by!**/*_test.go
📒 Files selected for processing (6)
- domain/cosmos/auth/types/types.go (1 hunks)
- domain/cosmos/tx/tx.go (1 hunks)
- domain/mocks/auth.go (1 hunks)
- domain/mocks/keyring.go (1 hunks)
- domain/mocks/txfees_client.go (1 hunks)
- domain/mocks/txservice_client.go (1 hunks)
🧰 Additional context used
🪛 GitHub Check: test_prices
domain/cosmos/auth/types/types.go
[failure] 43-43:
undefined: http.Get
🪛 GitHub Check: test_routes
domain/cosmos/auth/types/types.go
[failure] 43-43:
undefined: http.Get
🪛 GitHub Check: Run linter
domain/cosmos/auth/types/types.go
[failure] 1-1:
: # github.com/osmosis-labs/sqs/domain/cosmos/auth/types
[failure] 43-43:
undefined: http.Get (typecheck)
[failure] 43-43:
undefined: http.Get) (typecheck)domain/mocks/auth.go
[failure] 6-6:
could not import github.com/osmosis-labs/sqs/domain/cosmos/auth/types (-: # github.com/osmosis-labs/sqs/domain/cosmos/auth/types
🪛 GitHub Check: test_coingecko_prices
domain/cosmos/auth/types/types.go
[failure] 43-43:
undefined: http.Get
🪛 GitHub Check: test
domain/cosmos/auth/types/types.go
[failure] 43-43:
undefined: http.Get
🪛 GitHub Check: build
domain/cosmos/tx/tx.go
[failure] 7-7:
no required module provides package github.com/osmosis-labs/sqs/delivery/grpc; to add it:
🔇 Additional comments (13)
domain/mocks/auth.go (2)
9-9: Excellent use of compile-time interface check.The use of a blank identifier to verify that
AuthQueryClientMocksatisfies theauthtypes.QueryClientinterface is a good practice. It ensures type compatibility at compile-time without incurring any runtime cost.
1-7:⚠️ Potential issueVerify the import path for the custom
authtypespackage.The linter is unable to resolve the import for
github.com/osmosis-labs/sqs/domain/cosmos/auth/types. This could be due to the package not being in the expected location or a misconfiguration in the project structure.To verify the package location and structure, run the following script:
If the package is not found or the
QueryClientinterface is not defined, you may need to create the package or adjust the import path.✅ Verification successful
Import path for
authtypesis correct and resolvable.The
github.com/osmosis-labs/sqs/domain/cosmos/auth/typespackage exists and contains the necessaryQueryClientinterface. No issues found with the import.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and structure of the authtypes package # Test 1: Check if the package directory exists echo "Checking for package directory:" fd -t d -p "domain/cosmos/auth/types$" # Test 2: List Go files in the package directory echo "Listing Go files in the package directory:" fd -e go -p "domain/cosmos/auth/types" # Test 3: Check for QueryClient interface definition echo "Searching for QueryClient interface definition:" rg --type go "type QueryClient interface" -A 5Length of output: 989
🧰 Tools
🪛 GitHub Check: Run linter
[failure] 6-6:
could not import github.com/osmosis-labs/sqs/domain/cosmos/auth/types (-: # github.com/osmosis-labs/sqs/domain/cosmos/auth/typesdomain/mocks/keyring.go (3)
1-11: LGTM: Package declaration and imports are appropriate.The package name 'mocks' is suitable for a mock implementation. All imports are relevant to the functionality being implemented, and there are no unused imports.
13-13: LGTM: Proper interface assertion.The interface assertion
var _ keyring.Keyring = &Keyring{}is a good practice. It ensures at compile-time that theKeyringstruct implements thekeyring.Keyringinterface.
1-45: Summary of review for domain/mocks/keyring.goOverall, this file provides a good foundation for a mock Keyring implementation. The use of function fields for flexible mocking is a strong point. However, there are several areas for improvement:
- Add comments to struct fields for better documentation.
- Enhance error handling in mock methods, possibly returning errors instead of panicking.
- Improve the
GenPrivKeymethod by handling errors, validating input, and aligning it with the mock pattern used in other methods.- Consider adding unit tests for this mock implementation to ensure its correctness and catch any regressions in future changes.
Addressing these points will significantly improve the robustness and maintainability of this mock implementation.
domain/mocks/txfees_client.go (3)
1-9: LGTM: Package declaration and imports are appropriate.The package name
mocksis suitable for a mock implementation, and the imports are relevant to the functionality being implemented.
11-11: Excellent: Interface implementation check is in place.The line
var _ txfeestypes.QueryClient = &TxFeesQueryClient{}ensures at compile-time thatTxFeesQueryClientimplements thetxfeestypes.QueryClientinterface. This is a crucial check that prevents potential runtime errors due to incomplete interface implementation.
36-48: LGTM: DenomPoolId and BaseDenom methods are correctly implemented.These methods maintain the consistent implementation pattern seen in the previous methods. They correctly use the function fields and pass through all parameters.
As mentioned in the previous comment, consider using a helper function to reduce code duplication across all methods. This suggestion applies to these methods as well.
domain/mocks/txservice_client.go (3)
1-9: LGTM: Package declaration and imports are appropriate.The package name
mocksis suitable for this mock implementation. The imports are relevant and correctly include the necessary packages for the mock's functionality. The use of an alias fortxtypesis a good practice for clarity.
11-11: Excellent use of compile-time interface check.The line
var _ txtypes.ServiceClient = &ServiceClient{}is a valuable compile-time check. It ensures that theServiceClientstruct implements all methods required by thetxtypes.ServiceClientinterface. This practice helps catch any discrepancies early in the development process.
1-86: Overall, well-implemented mock with room for enhancements.This mock implementation of
txtypes.ServiceClientis well-structured, consistent, and serves its purpose effectively. The use of function fields for each method allows for flexible mocking, and the compile-time interface check ensures completeness.To recap the main suggestions for improvement:
- Add a descriptive comment for the
ServiceClientstruct to explain its purpose and usage.- Enhance the mock's flexibility by returning errors instead of panicking for unimplemented methods.
- Implement a call tracking mechanism to allow for more comprehensive testing scenarios.
These enhancements would make the mock more robust, safer to use, and provide additional capabilities for thorough testing.
Great job on implementing this mock! With these suggested improvements, it will become an even more valuable tool for testing the system's interaction with the Cosmos SDK transaction service.
domain/cosmos/tx/tx.go (2)
81-87: Verify signing parameters and avoid redundancyWhen signing the transaction:
signed, err := txclient.SignWithPrivKey( ctx, signingtypes.SignMode_SIGN_MODE_DIRECT, signerData, txBuilder, privKey, encodingConfig.TxConfig, account.Sequence)Ensure that the
account.Sequenceused here matches the one used in building the transaction. Also, sinceSignWithPrivKeysets the signature, there's no need to set it again unless modifications are made afterward.
208-209: Ensure accurate fee calculationIn
CalculateFeeAmount:return baseFee.MulInt64(int64(gas)).Ceil().TruncateInt()Using both
Ceil()andTruncateInt()might introduce rounding issues. Verify that this calculation accurately reflects the intended fee without precision loss.You might simplify the calculation:
-return baseFee.MulInt64(int64(gas)).Ceil().TruncateInt() +return baseFee.MulInt64(int64(gas)).Ceil().RoundInt()Using
RoundInt()ensures proper rounding.
Requested changes
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (7)
domain/mocks/auth.go (2)
22-26: Excellent helper method for mock setup.The
WithGetAccountmethod is a great addition for easy mock setup in tests. It follows a fluent interface pattern, which enhances readability in test code.Consider returning the mock instance to allow method chaining:
-func (m *AuthQueryClientMock) WithGetAccount(response *authtypes.QueryAccountResponse, err error) { +func (m *AuthQueryClientMock) WithGetAccount(response *authtypes.QueryAccountResponse, err error) *AuthQueryClientMock { m.GetAccountFunc = func(ctx context.Context, address string) (*authtypes.QueryAccountResponse, error) { return response, err } + return m }This change would allow for more concise test setup when multiple mock methods are added in the future.
1-26: Overall, well-implemented mock for QueryClient.The
AuthQueryClientMockprovides a solid foundation for mocking theQueryClientinterface. It follows good practices for Go mocks and offers flexibility in test setups. The suggested improvements (handling unmocked cases without panicking and enabling method chaining) would further enhance its usability.As this is part of a new feature (BE-594/domain/cosmos), ensure that comprehensive tests are written to validate the behavior of systems using this mock.
Would you like assistance in writing test cases that demonstrate the usage of this mock?
🧰 Tools
🪛 GitHub Check: Run linter
[failure] 6-6:
could not import github.com/osmosis-labs/sqs/domain/cosmos/auth/types (-: # github.com/osmosis-labs/sqs/domain/cosmos/auth/typesdomain/mocks/gas_calculator.go (2)
9-11: LGTM: GasCalculator struct is well-defined.The
GasCalculatorstruct is clear and flexible. TheCalculateGasFuncfield allows for easy customization of the mock's behavior.Consider adding a comment to explain the purpose of the
GasCalculatorstruct and itsCalculateGasFuncfield. This would improve the code's documentation and make it easier for other developers to understand and use this mock.
21-25: LGTM: WithCalculateGas method is well-implemented.The
WithCalculateGasmethod provides a clean way to configure the mock's behavior. Its implementation is correct and follows common patterns for mock objects.Consider adding a comment to explain that this method is used for setting up the mock's behavior in tests. For example:
// WithCalculateGas configures the mock to return the specified response, gas amount, and error // when CalculateGas is called. This method is typically used in test setup. func (m *GasCalculator) WithCalculateGas(response *txtypes.SimulateResponse, n uint64, err error) { // ... (existing implementation) }domain/cosmos/tx/gas.go (1)
17-21: Consider renamingclientCtxtoconnfor clarityThe parameter and struct field
clientCtxcould be misleading, as it's of typegogogrpc.ClientConn, not a client context. Renaming it toconnorgrpcConncan improve readability.Apply the following changes:
In
NewGasCalculatorfunction (Lines 17-21):-func NewGasCalculator(clientCtx gogogrpc.ClientConn) GasCalculator { - return &TxGasCalculator{ - clientCtx: clientCtx, +func NewGasCalculator(conn gogogrpc.ClientConn) GasCalculator { + return &TxGasCalculator{ + conn: conn, } }In
TxGasCalculatorstruct (Line 25):- clientCtx gogogrpc.ClientConn + conn gogogrpc.ClientConnIn
CalculateGasmethod (Line 34):- c.clientCtx, + c.conn,Also applies to: 25-25, 34-34
domain/mocks/txservice_client.go (1)
13-23: Add documentation comments to the struct and its fields for better clarity.Including GoDoc comments for the
TxServiceClientstruct and its function fields will enhance readability and maintainability. This helps other developers understand the purpose and usage of each method in the mock client.Example:
// TxServiceClient is a mock implementation of txtypes.ServiceClient for testing purposes. // Customize behaviors by setting the function fields accordingly. type TxServiceClient struct { // SimulateFunc simulates a transaction without broadcasting. SimulateFunc func(ctx context.Context, in *txtypes.SimulateRequest, opts ...grpc.CallOption) (*txtypes.SimulateResponse, error) // GetTxFunc retrieves a transaction by hash. GetTxFunc func(ctx context.Context, in *txtypes.GetTxRequest, opts ...grpc.CallOption) (*txtypes.GetTxResponse, error) // BroadcastTxFunc broadcasts a transaction to the network. BroadcastTxFunc func(ctx context.Context, in *txtypes.BroadcastTxRequest, opts ...grpc.CallOption) (*txtypes.BroadcastTxResponse, error) // Add comments for other function fields... }domain/cosmos/tx/tx.go (1)
143-152: Consider handling multiple signaturesThe
BuildSignaturesfunction is designed for single signatures:func BuildSignatures(publicKey cryptotypes.PubKey, signature []byte, sequence uint64) signingtypes.SignatureV2 { return signingtypes.SignatureV2{ PubKey: publicKey, Data: &signingtypes.SingleSignatureData{ SignMode: signingtypes.SignMode_SIGN_MODE_DIRECT, Signature: signature, }, Sequence: sequence, } }If you plan to support multi-signature accounts in the future, consider implementing support for
MultiSignatureDatato make your code more extensible.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
domain/cosmos/tx/tx_test.gois excluded by!**/*_test.go
📒 Files selected for processing (7)
- domain/cosmos/tx/gas.go (1 hunks)
- domain/cosmos/tx/tx.go (1 hunks)
- domain/mocks/auth.go (1 hunks)
- domain/mocks/gas_calculator.go (1 hunks)
- domain/mocks/keyring.go (1 hunks)
- domain/mocks/txfees_client.go (1 hunks)
- domain/mocks/txservice_client.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- domain/mocks/txfees_client.go
🧰 Additional context used
🪛 GitHub Check: Run linter
domain/mocks/auth.go
[failure] 6-6:
could not import github.com/osmosis-labs/sqs/domain/cosmos/auth/types (-: # github.com/osmosis-labs/sqs/domain/cosmos/auth/types
🔇 Additional comments (13)
domain/mocks/auth.go (3)
9-9: Excellent use of interface compliance check.The use of a blank identifier with type assertion
var _ authtypes.QueryClient = &AuthQueryClientMock{}is a great practice. It ensures at compile-time thatAuthQueryClientMockimplements theauthtypes.QueryClientinterface.
11-13: Well-designed mock struct.The
AuthQueryClientMockstruct is well-designed with a function fieldGetAccountFunc. This approach allows for flexible mocking of theGetAccountmethod in tests.
1-7: Verify the import path for theauthtypespackage.The static analysis tool reported an issue with importing the
authtypespackage. This could be due to the package not being present in the expected location or a misconfiguration in the project structure.Please ensure that the
authtypespackage exists at the specified path. You can run the following command to verify the package location:If the package is not found, you may need to create it or adjust the import path.
Would you like assistance in creating the
authtypespackage or adjusting the import path?✅ Verification successful
Import Path Verified.
The
authtypespackage exists at the specified path and contains the necessary Go files.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the authtypes package # Test: Check if the package directory exists fd -t d "auth/types$" --full-path ./domain/cosmos # Test: Check for Go files in the package fd -e go . --full-path ./domain/cosmos/auth/typesLength of output: 208
🧰 Tools
🪛 GitHub Check: Run linter
[failure] 6-6:
could not import github.com/osmosis-labs/sqs/domain/cosmos/auth/types (-: # github.com/osmosis-labs/sqs/domain/cosmos/auth/typesdomain/mocks/gas_calculator.go (2)
1-7: LGTM: Package declaration and imports are correct.The package name
mocksis appropriate for this mock implementation, and the imports from the Cosmos SDK are relevant to the file's purpose.
1-25: Overall, the GasCalculator mock is well-implemented.The
GasCalculatormock provides a flexible way to simulate gas calculations in tests. The implementation follows good practices for mock objects and allows for easy configuration of behavior.Key points:
- The struct and method signatures are appropriate for the intended use.
- The
WithCalculateGasmethod provides a clean way to set up the mock's behavior.- Error handling in the
CalculateGasmethod could be improved (see previous comment).- Additional comments would enhance the code's documentation.
These suggested improvements are minor and don't affect the core functionality. The implementation is solid and ready for use in tests.
domain/mocks/keyring.go (3)
1-11: LGTM: Package declaration and imports are appropriate.The package name "mocks" is suitable for a mock implementation, and the imports include necessary packages for the Keyring functionality.
13-19: LGTM: Keyring struct and interface assertion are well-defined.The interface assertion on line 13 is a good practice to ensure the mock implements the required
keyring.Keyringinterface. TheKeyringstruct with function fields allows for flexible mocking of different methods.
1-57: Overall assessment: Good mock implementation with room for improvement.The
Keyringmock implementation provides a good foundation for testing. However, there are several areas where it can be improved:
- Consistent error handling across all methods instead of panicking.
- Use of a custom error type for unimplemented methods.
- Alignment of the
GenPrivKeymethod with the mock pattern used in other methods.- Proper error handling and input validation in the
GenPrivKeymethod.Implementing these suggestions will result in a more robust and consistent mock implementation, enhancing its usefulness in testing scenarios.
domain/mocks/txservice_client.go (1)
25-92: Avoid panicking in mock methods; return errors instead.Panicking when a method is unimplemented can cause unexpected test failures. Returning a default error makes the mock safer and more flexible in tests where not all methods are needed.
Refer to the previous review comment by
coderabbitai[bot]for a suggested refactoring to return errors and optionally track method calls.domain/cosmos/tx/tx.go (4)
87-90: Handle potential errors when setting final signaturesAfter signing the transaction, the signatures are set:
err = txBuilder.SetSignatures(signed) if err != nil { return nil, err }Ensure that the error handling here is appropriate. If
txBuilder.SetSignatures(signed)fails, consider logging the error or providing more context to aid debugging.
127-127: Review the gas adjustment factorThe gas adjustment factor is set to
1.05:txFactory = txFactory.WithGasAdjustment(1.05)Ensure that this factor is sufficient for your application's needs. Depending on network conditions, a higher adjustment may be necessary to prevent transactions from failing due to underestimating gas.
164-180: Check for fee calculation errors and provide fallback optionsIn
CalculateFeeCoin, errors from querying base denomination or base fee result in returning an empty coin and error:if err != nil { return sdk.Coin{}, err }Consider providing a fallback fee or defaulting to a standard fee if the query fails. This enhances the robustness of your application by allowing transactions to proceed even if the fee query encounters issues.
185-185:⚠️ Potential issueHandle potential overflow in fee amount calculation
In
CalculateFeeAmount, the fee amount is calculated:return baseFee.MulInt64(int64(gas)).Ceil().TruncateInt()Ensure that the multiplication does not cause an integer overflow, especially if
gasvalues are large.Run the following script to verify the maximum gas limit used in the codebase:
domain/cosmos/auth/types/types.go
Outdated
| // QueryAccountResponse encapsulates the response for an account query. | ||
| type QueryAccountResponse struct { | ||
| Account Account `json:"account"` | ||
| } | ||
|
|
||
| // GetAccount retrieves account information for a given address. | ||
| func (c *queryClient) GetAccount(ctx context.Context, address string) (*QueryAccountResponse, error) { | ||
| resp, err := http.Get(ctx, c.lcd+"/cosmos/auth/v1beta1/accounts/"+address) |
There was a problem hiding this comment.
I'm also curious if we could rely more on cosmos SDK client helpers, potentially minimizing reimplementation.
Please take a look at this CosmosSender for inspiration:
https://github.com/polaris-portal/polaris/blob/ebafb78bf432380072c9f74a433cf61e7336ed83/app/gasstation/senders/cosmos.go#L46
There was a problem hiding this comment.
Yes! Great point, thanks! I have also got rid of HTTP, and switched to gRPC, it was a bit unclear how to fill all the fields for BaseAccount, leaving some fields not filled would make this client really confusing.
After the changes our local implementation is a wrapper around QueryClient, that in turn required me to pull in encodingConfig to be able to unpack response.
Let me know if that makes sense!
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
domain/cosmos/auth/types/types.go (1)
54-54: Include the actual account type in the error message for better debuggingEnhancing the error message by including the actual type received can help with debugging when the type assertion fails.
Apply this diff:
-return nil, fmt.Errorf("account is not of the type of BaseAccount") +return nil, fmt.Errorf("account is not of type *BaseAccount, got %T", account)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
domain/cosmos/tx/tx_test.gois excluded by!**/*_test.go
📒 Files selected for processing (3)
- domain/cosmos/auth/types/types.go (1 hunks)
- domain/cosmos/tx/tx.go (1 hunks)
- domain/mocks/auth.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- domain/cosmos/tx/tx.go
- domain/mocks/auth.go
🧰 Additional context used
This PR was opened as part of bigger claimbot feature to keep PRs small, focused and more manageable and adds
cosmos/auth,cosmos/txpackages containing various high level helper functions and wrappers abstracting lower level details interacting with the chain.cosmos/txpackage is used directly by claimbot to contruct and broadcast transactions.cosmos/authpackage is also used directly by claimbot to retrieve account number and sequence.Note, this PR will be merged after #525 is merged since it depends on
delivery/httppackage.Summary by CodeRabbit
New Features
Bug Fixes
Documentation