refactor: decouple base fee fetching as part of quote from simulation #550
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request involve updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RouterHandler
participant QuoteSimulator
participant MsgSimulator
Client->>RouterHandler: Request optimal quote
RouterHandler->>QuoteSimulator: SimulateQuote()
QuoteSimulator->>MsgSimulator: PriceMsgs()
MsgSimulator-->>QuoteSimulator: TxFeeInfo
QuoteSimulator-->>RouterHandler: TxFeeInfo
RouterHandler-->>Client: Return optimal quote
Possibly related PRs
Poem
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: 6
🧹 Outside diff range and nitpick comments (5)
domain/mocks/msg_simulator_mock.go (1)
41-42: Good architectural direction with structured return typeThe transition to
domain.QuotePriceInforepresents a positive architectural change:
- Encapsulates both success and error cases in a structured type
- Supports the new optimistic simulation approach
- Provides a more maintainable and type-safe interface
Consider documenting this pattern in the architecture documentation to ensure consistent usage across the codebase.
Also applies to: 77-78
quotesimulator/quote_simulator.go (1)
69-69: Consider wrapping the internal error message.Direct exposure of internal error messages could leak sensitive information. Consider wrapping it with a user-friendly message.
-return domain.QuotePriceInfo{Err: err.Error()} +return domain.QuotePriceInfo{Err: fmt.Sprintf("failed to retrieve account: %v", err)}CHANGELOG.md (1)
40-43: LGTM with a minor suggestion for clarity.The changelog entries accurately reflect the changes mentioned in the PR objectives and AI summary. However, consider making the #548 entry more descriptive to better explain the optimistic simulation behavior:
-- #548 - Return base fee in /quote regardless of simulation success. +- #548 - Make /quote endpoint return base fee even when simulation fails, enabling optimistic simulation behavior.domain/cosmos/tx/tx.go (1)
57-59: Consider renamingCalculateFeePricetoGetFeePricefor clarity.Since the function now retrieves fee price information without performing calculations, renaming it to
GetFeePriceorFetchFeePricemay improve readability and better convey its purpose.domain/cosmos/tx/msg_simulator.go (1)
95-97: Consider using theerrortype for theErrfield inQuotePriceInfoCurrently,
QuotePriceInfouses a string fieldErrto represent errors. Using theerrortype instead of a string would align with Go's idiomatic error handling practices, allowing for more robust and type-safe error management.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (4)
domain/cosmos/tx/msg_simulator_test.gois excluded by!**/*_test.godomain/cosmos/tx/tx_test.gois excluded by!**/*_test.goquotesimulator/quote_simulator_test.gois excluded by!**/*_test.gorouter/delivery/http/router_handler_test.gois excluded by!**/*_test.go
📒 Files selected for processing (9)
CHANGELOG.md(1 hunks)domain/cosmos/tx/msg_simulator.go(5 hunks)domain/cosmos/tx/tx.go(1 hunks)domain/mocks/msg_simulator_mock.go(3 hunks)domain/mocks/quote_simulator_mock.go(1 hunks)domain/quote_simulator.go(1 hunks)quotesimulator/quote_simulator.go(2 hunks)router/delivery/http/router_handler.go(1 hunks)router/usecase/routertesting/parsing/quote_amount_in_response_simulated.json(1 hunks)
🔇 Additional comments (12)
domain/mocks/quote_simulator_mock.go (2)
11-11: LGTM: Function signature aligns with the new QuotePriceInfo return type.
The updated signature correctly reflects the transition from multiple return values to the new QuotePriceInfo type, which better encapsulates both successful and failed simulation results.
Line range hint 15-20: LGTM: Mock implementation maintains proper behavior.
The implementation correctly:
- Preserves the delegation to SimulateQuoteFn when available
- Maintains the panic behavior for unimplemented cases
- Returns the new QuotePriceInfo type as expected
Let's verify that all implementations of QuoteSimulator have been updated to match this signature:
✅ Verification successful
All QuoteSimulator implementations correctly return QuotePriceInfo
The verification confirms that all implementations of SimulateQuote are consistent with the interface and return domain.QuotePriceInfo:
- Interface definition in
domain/quote_simulator.go - Main implementation in
quotesimulator/quote_simulator.go - Mock implementation in
domain/mocks/quote_simulator_mock.go - Test usage in
router/delivery/http/router_handler_test.go
All implementations use the same signature and return type, maintaining consistency across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all QuoteSimulator implementations use QuotePriceInfo
# Expected: All implementations should return QuotePriceInfo, not the old tuple
# Search for SimulateQuote method implementations
ast-grep --pattern 'func $_($_, quote domain.Quote, $_, $_) $ReturnType {
$$$
}'
Length of output: 84
Script:
#!/bin/bash
# Let's try a different approach to find QuoteSimulator implementations
# First, find all Go files
rg -t go "SimulateQuote.*Quote" -A 3
Length of output: 2123
router/usecase/routertesting/parsing/quote_amount_in_response_simulated.json (1)
55-57: LGTM! The new fields are properly structured.
The addition of base_fee and error fields to the price_info object aligns with the PR objective of returning the base fee regardless of simulation success. The decimal precision and type representations are consistent with the codebase standards.
domain/mocks/msg_simulator_mock.go (2)
12-12: LGTM: Import addition aligns with new return type
The addition of the domain package import is necessary to support the new QuotePriceInfo return type.
41-42: Verify test coverage for the new return type
The change from tuple return type to domain.QuotePriceInfo is aligned with the PR objective. However, we should verify that all tests using this mock have been updated accordingly.
Also applies to: 77-78
✅ Verification successful
Let me gather more information about the test implementations to verify if they've been properly updated for the new return type.
Let me try one more search to find the actual implementation and verify the test updates:
Tests have been properly updated for the new return type
Based on the evidence found:
- The mock implementation in
quotesimulator/quote_simulator_test.gocorrectly returnsdomain.QuotePriceInfo - The test in
domain/cosmos/tx/msg_simulator_test.goproperly handles the new return type, including error checking - All test files are using the updated
QuotePriceInfostruct which includesAdjustedGasUsed,FeeCoin,BaseFee, andErrfields - The actual implementation in
quotesimulator/quote_simulator.gocorrectly uses the new return type
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find tests that might be affected by the mock changes
# Look for test files using MsgSimulatorMock
echo "Finding test files using MsgSimulatorMock:"
rg -l "MsgSimulatorMock" --type go --type-add "go:*.go" | grep "_test.go"
# Look for specific usage of PriceMsgs in tests
echo -e "\nChecking PriceMsgs usage in tests:"
rg "PriceMsgs" --type go --type-add "go:*.go" -B 2 -A 2 | grep "_test.go" -A 4
Length of output: 1819
Script:
#!/bin/bash
# Check the implementation details of the test files using PriceMsgs
# Check quotesimulator test implementation
echo "=== Quote Simulator Test Implementation ==="
rg -A 15 "PriceMsgsFn: func" quotesimulator/quote_simulator_test.go
# Check msg simulator test implementation
echo -e "\n=== Msg Simulator Test Implementation ==="
rg -A 15 "func TestPriceMsgs" domain/cosmos/tx/msg_simulator_test.go
# Check the actual implementation for comparison
echo -e "\n=== Actual Implementation ==="
rg -A 10 "func \(s \*MsgSimulator\) PriceMsgs" domain/cosmos/tx/msg_simulator.go
Length of output: 1614
Script:
#!/bin/bash
# Search for PriceMsgs implementation and test assertions
echo "=== PriceMsgs Implementation ==="
rg "type QuotePriceInfo struct" -A 5 domain/
echo -e "\n=== Test Assertions ==="
# Look for test assertions related to PriceMsgs
rg "assert.*PriceMsgs|require.*PriceMsgs" --type go -B 2 -A 2
echo -e "\n=== Test Implementation Details ==="
# Get more context around the test implementation
rg "msgSimulator.PriceMsgs|PriceMsgsFn:" -B 5 -A 10 --type go
Length of output: 3461
quotesimulator/quote_simulator.go (3)
40-40: LGTM! Clear error handling with descriptive message.
The error handling is well-structured and maintains the validation logic while adapting to the new return type.
73-73: Consider validating PriceMsgs result.
While the direct return is clean, consider validating that essential fields (like BaseFee) are present in the returned QuotePriceInfo, especially since this is the main objective of the PR.
37-37: Verify API consumers are updated for the breaking change.
The return type change from (uint64, sdk.Coin, error) to domain.QuotePriceInfo is a breaking change. While this change aligns well with the PR objective, we should ensure all consumers are updated.
✅ Verification successful
All API consumers have been properly updated to handle the new return type
The search results show that all consumers of SimulateQuote are already updated to handle the new domain.QuotePriceInfo return type:
router/delivery/http/router_handler.go: Correctly uses the new return type and sets it viaquote.SetQuotePriceInfo(&priceInfo)router/delivery/http/router_handler_test.go: Mock implementation returns the newdomain.QuotePriceInfotypedomain/mocks/quote_simulator_mock.go: Mock interface is updated with the new return typequotesimulator/quote_simulator_test.go: Tests are updated to handle the new return type
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all direct callers of SimulateQuote to verify they handle the new return type
ast-grep --pattern 'SimulateQuote($$$)'
Length of output: 41
Script:
#!/bin/bash
# Let's try a more comprehensive search using ripgrep to find any calls to SimulateQuote
rg -l "SimulateQuote" --type go
# Then let's examine the content of these files
rg "SimulateQuote" -A 3 -B 3 --type go
Length of output: 5185
router/delivery/http/router_handler.go (1)
162-165: Verify error handling in simulation result
While the changes align with the PR objective to return the base fee regardless of simulation success, consider enhancing error visibility for better debugging and monitoring.
Consider adding error logging before setting the quote price info:
priceInfo := a.QuoteSimulator.SimulateQuote(ctx, quote, req.SlippageToleranceMultiplier, simulatorAddress)
+
+// Log simulation errors for monitoring while still proceeding with the base fee
+if priceInfo.Error != nil {
+ a.logger.Error("quote simulation failed", "error", priceInfo.Error)
+}
// Set the quote price info.
quote.SetQuotePriceInfo(&priceInfo)domain/cosmos/tx/tx.go (2)
62-62: Good handling of error cases with appropriate zero values.
Returning empty strings and zero-valued osmomath.Dec upon encountering errors is appropriate, ensuring that callers can handle errors without unintended side effects.
Also applies to: 67-67
70-70: Verify that all dependent functions handle the new return types correctly.
With the change from CalculateFeeCoin to CalculateFeePrice, ensure that any functions or methods relying on this function are updated to accommodate the new return types (string, osmomath.Dec, error).
Run the following script to identify usages of CalculateFeePrice and check for proper handling:
✅ Verification successful
Based on the search results, I can generate the final response as we have sufficient information about the usage of CalculateFeePrice.
All dependent functions correctly handle the new return types
The function CalculateFeePrice is used in two locations, and both handle the return types (string, osmomath.Dec, error) correctly:
domain/cosmos/tx/tx_test.go: Properly unpacks intodenom, baseFee, errand checks error conditionsdomain/cosmos/tx/msg_simulator.go: Correctly assignsbaseDenom, baseFee, errand handles the error case
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify dependent functions are updated to handle new return types of `CalculateFeePrice`.
# Expect: All usages should correctly handle `(string, osmomath.Dec, error)`.
rg 'CalculateFeePrice' --glob '!domain/cosmos/tx/tx.go' -A 5
Length of output: 800
domain/cosmos/tx/msg_simulator.go (1)
53-53: Verify all implementations and usages of MsgSimulator are updated to match the new PriceMsgs signature
The return type of the PriceMsgs method in the MsgSimulator interface has changed to domain.QuotePriceInfo. Ensure that all implementations of this interface and all callers of PriceMsgs are updated accordingly to prevent interface mismatch errors.
Run the following script to identify any implementations that may not have been updated:
✅ Verification successful
Based on the search results, I can now generate the final response:
All implementations and usages of PriceMsgs are correctly aligned with the new signature
The verification shows:
- The concrete implementation in
txGasCalulatorcorrectly returnsdomain.QuotePriceInfo - The mock implementation in
domain/mocks/msg_simulator_mock.gois auto-generated and will be updated accordingly - All usages of
PriceMsgsare properly handling the returnedQuotePriceInfo, particularly in:quotesimulator/quote_simulator.godomain/cosmos/tx/msg_simulator.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all implementations of MsgSimulator and ensure they return domain.QuotePriceInfo
# Search for implementations of PriceMsgs that do not return domain.QuotePriceInfo
ast-grep --lang go --pattern $'func ($_).PriceMsgs($$$) $_ {\n$$$\n}' --ignore 'domain.QuotePriceInfo' --output filename
Length of output: 379
Script:
#!/bin/bash
# Let's try a different approach to find all implementations of PriceMsgs
# First, find the interface definition
echo "=== MsgSimulator interface definition ==="
rg -A 5 "type MsgSimulator interface"
echo -e "\n=== PriceMsgs implementations ==="
# Find all PriceMsgs method implementations
ast-grep --pattern 'func ($x) PriceMsgs($$$) $ret {
$$$
}'
# Also search for any direct calls to PriceMsgs
echo -e "\n=== PriceMsgs method calls ==="
rg "PriceMsgs\(" -A 2
Length of output: 2564
| }, | ||
| "base_fee": "0.500000000000000000", | ||
| "error": "" |
There was a problem hiding this comment.
💡 Codebase verification
Additional test fixtures for error scenarios are needed
Based on the code analysis:
- The test files only cover successful scenarios with empty error strings
- The router handler tests include error cases for invalid inputs but not for simulation failures
- The quote tests focus on price impact and fee calculations but not error conditions
The review comment is correct - there are no test fixtures covering error scenarios. This is particularly important for the simulated quote response where the error field exists but is only tested with empty string values.
Suggested error scenarios to add:
- Simulation failure with base fee
- Invalid pool configuration
- Insufficient liquidity
- Price impact too high
🔗 Analysis chain
Consider adding test fixtures for error scenarios.
Since this fixture represents a successful case (empty error string), it would be valuable to add additional test fixtures that cover scenarios where:
- Simulation fails but base fee is still returned
- Various error messages are present
- Edge cases with different fee values
This would help ensure the new optimistic simulation behavior is thoroughly tested.
Let's check if error scenario fixtures exist:
Would you like me to help create these additional test fixtures?
This script will help us:
- Find all relevant JSON test fixtures
- Examine their error handling patterns
- Determine if error scenarios are adequately covered
This will help us understand:
- How the test fixtures are used in tests
- If there are any error scenarios being tested
- The overall test coverage for quote responses
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for error scenario test fixtures in the router testing directory
# Expected: Find fixtures with non-empty error fields or error in filename
# Search for JSON files that might contain error scenarios
fd . --extension json --exec grep -l "error.*[a-zA-Z]" {} \; | grep -i "quote\|error"
Length of output: 90
Script:
#!/bin/bash
# Let's try a different approach to find test fixtures and their error fields
# First, let's find all JSON files in the router testing directory
fd . --extension json | grep -i "router.*test\|quote"
# Then let's examine the content of these files to see error handling
for file in $(fd . --extension json | grep -i "router.*test\|quote"); do
echo "=== $file ==="
# Look for the error field and its surrounding context
grep -A 2 -B 2 '"error"' "$file" || true
done
Length of output: 3191
Script:
#!/bin/bash
# Let's examine the content of quote-related files more thoroughly
echo "=== quote_amount_in_response.json ==="
cat router/usecase/routertesting/parsing/quote_amount_in_response.json
echo -e "\n=== quote_amount_in_response_simulated.json ==="
cat router/usecase/routertesting/parsing/quote_amount_in_response_simulated.json
echo -e "\n=== quote_amount_out_response.json ==="
cat router/usecase/routertesting/parsing/quote_amount_out_response.json
# Let's also check if there are any test files that use these fixtures
fd . --extension go | grep -i "test" | xargs grep -l "quote.*response.*json"
Length of output: 5148
Script:
#!/bin/bash
# Let's examine the test files to understand how these fixtures are used
echo "=== router_handler_test.go ==="
cat router/delivery/http/router_handler_test.go
echo -e "\n=== quote_test.go ==="
cat router/usecase/quote_test.go
Length of output: 23078
domain/quote_simulator.go
Outdated
| @@ -14,10 +14,12 @@ type QuoteSimulator interface { | |||
| // - Only direct (non-split) quotes are supported. | |||
| // Retursn error if: | |||
| // - Simulator address does not have enough funds to pay for the quote. | |||
| SimulateQuote(ctx context.Context, quote Quote, slippageToleranceMultiplier osmomath.Dec, simulatorAddress string) (uint64, sdk.Coin, error) | |||
| SimulateQuote(ctx context.Context, quote Quote, slippageToleranceMultiplier osmomath.Dec, simulatorAddress string) QuotePriceInfo | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using the error type for Err in QuotePriceInfo
In Go, it's idiomatic to use the error type instead of a string to represent errors. This allows for richer error handling, including wrapping and unwrapping errors, and compatibility with Go's error interfaces. By changing the Err field to type error, you enhance the code's robustness and maintainability.
Apply the following diff to update the Err field and adjust the method signature accordingly:
type QuotePriceInfo struct {
AdjustedGasUsed uint64 `json:"adjusted_gas_used"`
FeeCoin sdk.Coin `json:"fee_coin"`
BaseFee osmomath.Dec `json:"base_fee"`
- Err string `json:"error"`
+ Err error `json:"error"`
}
- SimulateQuote(ctx context.Context, quote Quote, slippageToleranceMultiplier osmomath.Dec, simulatorAddress string) QuotePriceInfo
+ SimulateQuote(ctx context.Context, quote Quote, slippageToleranceMultiplier osmomath.Dec, simulatorAddress string) (QuotePriceInfo, error)This change ensures proper error propagation and adherence to Go best practices for error handling.
Also applies to: 24-24
domain/cosmos/tx/tx.go
Outdated
| // CalculateFeePrice determines the appropriate fee price for a transaction based on the current base fee | ||
| // and the amount of gas used. It queries the base denomination and EIP base fee using the provided gRPC connection. | ||
| func CalculateFeeCoin(ctx context.Context, client txfeestypes.QueryClient, gas uint64) (sdk.Coin, error) { | ||
| func CalculateFeePrice(ctx context.Context, client txfeestypes.QueryClient) (string, osmomath.Dec, error) { |
There was a problem hiding this comment.
Update the function documentation to accurately reflect its behavior.
The CalculateFeePrice function no longer uses the gas amount in its calculation, but the comment still mentions it. Please update the docstring to match the current functionality.
Apply this diff to correct the docstring:
-// CalculateFeePrice determines the appropriate fee price for a transaction based on the current base fee
-// and the amount of gas used. It queries the base denomination and EIP base fee using the provided gRPC connection.
+// CalculateFeePrice retrieves the base denomination and EIP base fee using the provided gRPC client.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // CalculateFeePrice determines the appropriate fee price for a transaction based on the current base fee | |
| // and the amount of gas used. It queries the base denomination and EIP base fee using the provided gRPC connection. | |
| func CalculateFeeCoin(ctx context.Context, client txfeestypes.QueryClient, gas uint64) (sdk.Coin, error) { | |
| func CalculateFeePrice(ctx context.Context, client txfeestypes.QueryClient) (string, osmomath.Dec, error) { | |
| // CalculateFeePrice retrieves the base denomination and EIP base fee using the provided gRPC client. | |
| func CalculateFeePrice(ctx context.Context, client txfeestypes.QueryClient) (string, osmomath.Dec, error) { |
domain/cosmos/tx/msg_simulator.go
Outdated
| baseDenom, baseFee, err := CalculateFeePrice(ctx, txfeesClient) | ||
| if err != nil { | ||
| return domain.QuotePriceInfo{Err: err.Error(), BaseFee: baseFee} | ||
| } |
There was a problem hiding this comment.
Ensure BaseFee is valid when CalculateFeePrice returns an error
When CalculateFeePrice encounters an error, baseFee may not be properly initialized. Returning a QuotePriceInfo with an uninitialized or invalid BaseFee could lead to incorrect fee calculations downstream. Consider handling this case by setting a default value or adjusting the error handling to prevent propagating potentially invalid data.
domain/cosmos/tx/msg_simulator.go
Outdated
| _, gasAdjusted, err := c.SimulateMsgs( | ||
| encodingConfig, | ||
| account, | ||
| chainID, | ||
| msg, | ||
| ) | ||
| if err != nil { | ||
| return 0, sdk.Coin{}, err | ||
| return domain.QuotePriceInfo{Err: err.Error(), BaseFee: baseFee} |
There was a problem hiding this comment.
Handle AdjustedGasUsed appropriately when SimulateMsgs fails
If SimulateMsgs returns an error, the value of gasAdjusted may not be valid. Returning it in QuotePriceInfo without verification could lead to misleading information. Ensure that AdjustedGasUsed is only set when SimulateMsgs succeeds, or clearly document its validity in error scenarios.
domain/cosmos/tx/msg_simulator.go
Outdated
| func (c *txGasCalulator) PriceMsgs(ctx context.Context, txfeesClient txfeestypes.QueryClient, encodingConfig cosmosclient.TxConfig, account *authtypes.BaseAccount, chainID string, msg ...sdk.Msg) domain.QuotePriceInfo { | ||
| baseDenom, baseFee, err := CalculateFeePrice(ctx, txfeesClient) | ||
| if err != nil { | ||
| return domain.QuotePriceInfo{Err: err.Error(), BaseFee: baseFee} | ||
| } | ||
|
|
||
| _, gasAdjusted, err := c.SimulateMsgs( | ||
| encodingConfig, | ||
| account, | ||
| chainID, | ||
| msg, | ||
| ) | ||
| if err != nil { | ||
| return 0, sdk.Coin{}, err | ||
| return domain.QuotePriceInfo{Err: err.Error(), BaseFee: baseFee} | ||
| } | ||
|
|
||
| feeCoin, err := CalculateFeeCoin(ctx, txfeesClient, gasAdjusted) | ||
| if err != nil { | ||
| return 0, sdk.Coin{}, err | ||
| } | ||
| feeAmount := CalculateFeeAmount(baseFee, gasAdjusted) | ||
|
|
||
| return gasAdjusted, feeCoin, nil | ||
| return domain.QuotePriceInfo{ | ||
| AdjustedGasUsed: gasAdjusted, | ||
| FeeCoin: sdk.Coin{Denom: baseDenom, Amount: feeAmount}, | ||
| BaseFee: baseFee, | ||
| Err: "", | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refining error handling within PriceMsgs
The current implementation returns QuotePriceInfo with partial data even when errors occur. This could lead to confusion about which fields are valid. Consider one of the following approaches:
- Option 1: Return a pointer to
QuotePriceInfoand set it tonilwhen an error occurs. - Option 2: Introduce a boolean flag indicating the validity of the data.
- Option 3: Separate successful and error responses to ensure clarity.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (11)
domain/base_fee.go (1)
5-9: Enhance struct documentation and consider adding validation methods.The struct implementation looks good, but consider these improvements:
- Enhance the struct documentation to be more descriptive:
-// BaseFee holds the denom and current base fee +// BaseFee represents the current base fee for a specific denomination. +// It is used across the application for fee calculations and routing logic.
- Add field documentation:
type BaseFee struct { + // Denom represents the denomination of the fee (e.g., "uosmo") Denom string + // CurrentFee represents the current base fee amount as a decimal CurrentFee osmomath.Dec }
- Consider adding validation methods to ensure the struct's integrity:
// Validate ensures the BaseFee fields contain valid values func (b BaseFee) Validate() error { if b.Denom == "" { return fmt.Errorf("denom cannot be empty") } if b.CurrentFee.IsNegative() { return fmt.Errorf("current fee cannot be negative") } return nil }ingest/usecase/plugins/basefee/base_fee_end_block_update_plugin.go (2)
20-26: Consider adding parameter validation.While the constructor is well-implemented, it would be more robust with nil checks for the required dependencies.
func NewEndBlockUpdatePlugin(routerRepository routerrepo.RouterRepository, txfeesClient txfeestypes.QueryClient, logger log.Logger) *baseFeeEndBlockUpdatePlugin { + if routerRepository == nil { + panic("routerRepository is required") + } + if txfeesClient == nil { + panic("txfeesClient is required") + } + if logger == nil { + panic("logger is required") + } return &baseFeeEndBlockUpdatePlugin{ routerRepository: routerRepository, txfeesClient: txfeesClient, logger: logger, } }
28-29: Remove unnecessary newline.There's an extra newline at the start of the function body.
// ProcessEndBlock calculates the base fee for the current block and updates the router repository with the new base fee. -func (p *baseFeeEndBlockUpdatePlugin) ProcessEndBlock(ctx context.Context, blockHeight uint64, metadata domain.BlockPoolMetadata) error { - +func (p *baseFeeEndBlockUpdatePlugin) ProcessEndBlock(ctx context.Context, blockHeight uint64, metadata domain.BlockPoolMetadata) error {🧰 Tools
🪛 GitHub Check: Run linter
[failure] 29-29:
unnecessary leading newline (whitespace)router/usecase/routertesting/parsing/quote_amount_in_response_base_fee.json (1)
2-5: Add token symbol comments for IBC denomsThe IBC denomination is not immediately clear what token it represents. Consider adding comments to indicate the actual token symbols (e.g., OSMO, ATOM, etc.) for better readability of the test data.
"amount_in": { + // e.g. OSMO token "denom": "ibc/EA1D43981D5C9A1C4AAEA9C23BB1D4FA126BA9BC7020A25E0AE4AA841EA25DC5", "amount": "10000000" },router/types/get_quote_request.go (2)
71-74: Consider setting an explicit default value for AppendBaseFee.While the current implementation is correct, setting an explicit default value for
AppendBaseFeebefore parsing would make the behavior more obvious when the parameter is not provided.func (r *GetQuoteRequest) UnmarshalHTTPRequest(c echo.Context) error { var err error + r.AppendBaseFee = false // Set explicit default r.AppendBaseFee, err = domain.ParseBooleanQueryParam(c, "appendBaseFee") if err != nil { return err }
22-22: Add documentation for the AppendBaseFee field.Consider adding a comment to document the purpose and behavior of the
AppendBaseFeefield, especially its relationship with simulation results.+ // AppendBaseFee indicates whether to return the base fee in the response + // regardless of simulation success. AppendBaseFee booldomain/mvc/router.go (1)
80-82: LGTM! Consider error handling strategy.The addition of
GetBaseFee()toRouterUsecaseinterface is well-placed and supports the PR objective. However, since this is a critical fee-related operation, consider whether error handling should be included in the interface definition.Consider updating the method signature to include error handling:
-GetBaseFee() domain.BaseFee +GetBaseFee() (domain.BaseFee, error)This would allow implementations to handle scenarios such as:
- Configuration errors
- Invalid fee data
- Database access issues (if fees are stored externally)
domain/mocks/router_usecase_mock.go (1)
42-44: Consider adding a customizable function field for GetBaseFeeFor consistency with other methods in this mock, consider adding a
GetBaseFeeFuncfield to allow tests to customize the behavior if needed.Here's the suggested implementation:
type RouterUsecaseMock struct { + GetBaseFeeFunc func() domain.BaseFee // ... other fields ... BaseFee domain.BaseFee } func (m *RouterUsecaseMock) GetBaseFee() domain.BaseFee { + if m.GetBaseFeeFunc != nil { + return m.GetBaseFeeFunc() + } return m.BaseFee }app/sidecar_query_server.go (1)
327-330: Consider making base fee fetcher plugin configurableWhile the unconditional registration ensures base fee is always available, consider making it configurable like other plugins for consistency. This would:
- Follow the same pattern as other plugins
- Allow disabling in environments where base fee might not be needed
- Make the behavior more explicit in the configuration
Example configuration structure:
type BaseFeeFetcherConfig struct { Enabled bool `yaml:"enabled"` }domain/cosmos/tx/msg_simulator.go (2)
52-52: Update the documentation forPriceMsgsto reflect the new return typeThe
PriceMsgsmethod now returns adomain.QuotePriceInfo, but the existing comment still refers to the old return values (gas usedandfee coin). Please update the function documentation to accurately describe the new return type and its fields.
55-56: Correct the function comment forNewMsgSimulatorThe comment states
// NewMsgSimulator creates a new GasCalculator instance, but the function actually creates a newMsgSimulator. Please update the comment to correctly reflect the function's purpose.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (5)
domain/cosmos/tx/msg_simulator_test.gois excluded by!**/*_test.godomain/cosmos/tx/tx_test.gois excluded by!**/*_test.goingest/usecase/plugins/orderbook/claimbot/tx_test.gois excluded by!**/*_test.goquotesimulator/quote_simulator_test.gois excluded by!**/*_test.gorouter/delivery/http/router_handler_test.gois excluded by!**/*_test.go
📒 Files selected for processing (18)
app/sidecar_query_server.go(3 hunks)domain/base_fee.go(1 hunks)domain/cosmos/tx/msg_simulator.go(6 hunks)domain/cosmos/tx/tx.go(2 hunks)domain/mocks/msg_simulator_mock.go(3 hunks)domain/mocks/router_usecase_mock.go(1 hunks)domain/mvc/router.go(2 hunks)domain/quote_simulator.go(1 hunks)ingest/usecase/plugins/basefee/base_fee_end_block_update_plugin.go(1 hunks)ingest/usecase/plugins/orderbook/claimbot/config.go(1 hunks)ingest/usecase/plugins/orderbook/claimbot/tx.go(1 hunks)quotesimulator/quote_simulator.go(2 hunks)router/delivery/http/router_handler.go(1 hunks)router/repository/memory_router_repository.go(2 hunks)router/types/get_quote_request.go(2 hunks)router/usecase/router_usecase.go(1 hunks)router/usecase/routertesting/parsing/quote_amount_in_response_base_fee.json(1 hunks)router/usecase/routertesting/parsing/quote_amount_in_response_simulated.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- domain/quote_simulator.go
- quotesimulator/quote_simulator.go
- router/delivery/http/router_handler.go
- router/usecase/routertesting/parsing/quote_amount_in_response_simulated.json
🧰 Additional context used
🪛 GitHub Check: Run linter
ingest/usecase/plugins/basefee/base_fee_end_block_update_plugin.go
[failure] 29-29:
unnecessary leading newline (whitespace)
🔇 Additional comments (23)
ingest/usecase/plugins/basefee/base_fee_end_block_update_plugin.go (2)
14-18: LGTM! Well-structured plugin definition.
The struct follows good design practices with clear interface-based dependencies and proper dependency injection support.
41-41: LGTM! Good practice with interface assertion.
The compile-time interface assertion ensures the struct properly implements EndBlockProcessPlugin.
ingest/usecase/plugins/orderbook/claimbot/config.go (2)
48-51: LGTM on the overall configuration changes.
The changes align with the PR objective to handle base fees independently of simulation success. The configuration structure maintains clean separation of concerns with clear dependency injection.
48-48: Please document the purpose of the nil parameter.
The change from NewGasCalculator to NewMsgSimulator includes an additional nil parameter. Could you clarify its purpose and document when it should be non-nil?
Let's verify the consistent usage of this new constructor across the codebase:
router/usecase/routertesting/parsing/quote_amount_in_response_base_fee.json (2)
47-55: Verify if the high price impact is intentional
The price impact of -56.54% seems unusually high for a typical transaction. If this is meant to be a regular test case rather than an edge case, consider using more realistic values that reflect common trading scenarios.
Additionally, the relationship between fee_coin.amount: "0" and base_fee: "0.5" should be documented to clarify the intended behavior.
Let's check the context of these values:
#!/bin/bash
# Search for price impact calculations and documentation
rg -g '!*.json' "price_impact" -A 57-46: Consider adding edge cases to test data
While this test case covers a multi-hop route with different spread factors and taker fees, consider adding additional test files for edge cases:
- Single pool route
- Maximum allowed hops
- Failed simulation scenarios (since PR aims to return base fee regardless of simulation success)
Let's check if these test cases exist:
domain/mocks/msg_simulator_mock.go (2)
11-11: LGTM: Return type update aligns with interface changes.
The changes correctly update the mock to use the new domain.QuotePriceInfo return type, maintaining consistency with the MsgSimulator interface changes.
Also applies to: 39-39
70-74: Verify mock implementation usage across tests.
The mock implementation has been updated to match the interface changes. However, we should verify that all test cases using this mock have been updated accordingly.
Also applies to: 76-76
✅ Verification successful
Let me correct the syntax error in the previous script and try again to find the test files and their usage of MsgSimulatorMock.
Let me try one more time with the correct syntax to find test files and their usage patterns.
Mock implementation is correctly used across tests
Based on the search results, all usages of PriceMsgs are consistent with the updated interface:
- The mock implementation in
domain/mocks/msg_simulator_mock.gocorrectly returnsdomain.QuotePriceInfo - The test file
domain/cosmos/tx/msg_simulator_test.gouses the mock appropriately - The implementation in
quotesimulator/quote_simulator.gocorrectly handles the return type - All callers properly handle the return value including error cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all test files using MsgSimulatorMock to ensure they're updated
# for the new QuotePriceInfo return type
# Search for test files using MsgSimulatorMock
echo "Searching for test files using MsgSimulatorMock..."
rg -l "MsgSimulatorMock" --type go --test-file
# Search for PriceMsgs calls in test files to verify return type handling
echo -e "\nChecking PriceMsgs usage in tests..."
rg "PriceMsgs\(" --type go --test-file -A 5
Length of output: 1120
Script:
#!/bin/bash
# Search for test files using MsgSimulatorMock
echo "Searching for test files using MsgSimulatorMock..."
rg -l "MsgSimulatorMock" ".*_test\.go$"
# Search for PriceMsgs calls in test files
echo -e "\nChecking PriceMsgs usage in tests..."
rg "PriceMsgs\(" ".*_test\.go$" -A 5
Length of output: 384
Script:
#!/bin/bash
# First find all test files
echo "Finding all test files using MsgSimulatorMock..."
fd -e go -t f test$ | xargs rg -l "MsgSimulatorMock"
echo -e "\nChecking PriceMsgs usage patterns..."
fd -e go -t f test$ | xargs rg "PriceMsgs\(" -B 2 -A 3
Length of output: 3545
domain/cosmos/tx/tx.go (2)
9-9: LGTM!
The new domain package import is necessary for the BaseFee type usage.
60-74: LGTM! The refactoring aligns well with the PR objectives.
The changes correctly implement the new behavior:
- Simplified function that focuses solely on retrieving base fee information
- Proper error handling with appropriate empty BaseFee returns
- Thread-safe implementation using the provided context
The docstring needs updating as mentioned in the previous review.
ingest/usecase/plugins/orderbook/claimbot/tx.go (2)
Line range hint 1-142: Implementation looks solid!
The overall implementation demonstrates good practices:
- Clear separation of concerns
- Proper error handling
- Flexible design supporting testing through encoding config customization
- Well-structured transaction building and sending process
71-74: Verify the removal of txfeesClient parameter's impact on fee handling.
The removal of txfeesClient from BuildTx aligns with the PR's objective to refactor fee handling. However, let's verify that fee calculation is properly handled in the new implementation.
router/types/get_quote_request.go (1)
22-22: LGTM! Field addition is well-structured.
The new AppendBaseFee boolean field is appropriately placed and follows Go naming conventions.
router/repository/memory_router_repository.go (3)
30-32: LGTM: Interface changes are well-defined.
The new methods SetBaseFee and GetBaseFee are appropriately added to the RouterRepository interface, aligning with the PR's objective to handle base fee information.
50-62: LGTM: Thread-safe implementation of base fee methods.
The implementation demonstrates proper use of RWMutex with:
- Read lock for GetBaseFee
- Write lock for SetBaseFee
- Appropriate use of defer for unlocking
Line range hint 30-62: Verify integration with simulation logic.
Let's ensure that the router repository's base fee implementation is properly integrated with the simulation logic to fulfill the PR's objective of returning base fee regardless of simulation success.
✅ Verification successful
Base fee implementation is properly integrated with simulation logic
The verification shows that the base fee implementation in memory_router_repository.go is correctly integrated:
- The base fee is properly used in quote generation through
router_handler.goandrouter_usecase.go - The simulation logic in
msg_simulator.gocorrectly handles base fee:- Validates base fee before simulation (checks for nil/zero values)
- Returns base fee in quote response regardless of simulation success
- Uses base fee for fee calculation after successful simulation
- Base fee is maintained by the
baseFeeEndBlockUpdatePluginwhich updates it through the repository interface
The thread-safe implementation ensures consistent base fee access across these integration points.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the integration of RouterRepository's base fee with simulation logic
# Test 1: Check if the base fee is used in simulation/quote logic
echo "Checking base fee usage in simulation/quote logic..."
rg -l "GetBaseFee|SetBaseFee" --type go
# Test 2: Look for error handling patterns around base fee
echo "Checking error handling patterns..."
rg "BaseFee.*error|error.*BaseFee" --type go
# Test 3: Verify quote endpoint implementation
echo "Checking quote endpoint implementation..."
rg "/quote.*BaseFee|BaseFee.*/quote" --type go
Length of output: 1555
Script:
#!/bin/bash
# Let's analyze the integration points more deeply
# Check how base fee is used in router handler and usecase
echo "Checking router handler implementation..."
rg -A 10 "GetBaseFee" router/delivery/http/router_handler.go router/usecase/router_usecase.go
# Check the simulation logic integration
echo "Checking simulation logic..."
rg -A 10 "BaseFee" domain/cosmos/tx/msg_simulator.go
# Check the base fee plugin implementation
echo "Checking base fee plugin..."
rg -A 10 "BaseFee" ingest/usecase/plugins/basefee/base_fee_end_block_update_plugin.go
# Check the router repository interface definition
echo "Checking router repository interface..."
rg -A 5 "RouterRepository" domain/mvc/router.go
Length of output: 3689
domain/mvc/router.go (1)
43-44: LGTM! Verify implementation consistency.
The addition of GetBaseFee() to RouterRepository interface aligns well with its existing fee management responsibilities.
Let's verify that all implementations of this interface properly implement the new method:
✅ Verification successful
Implementation verified in repository layer
The verification shows that:
- The
GetBaseFee()method is properly implemented inrouter/repository/memory_router_repository.go - This is the only implementation of
RouterRepositoryinterface in the codebase - The implementation returns the correct type
domain.BaseFee
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for implementations of RouterRepository interface and verify GetBaseFee() implementation
# Find all potential implementations
ast-grep --pattern 'type $_ struct {
$$$
}
func ($_ $_) GetBaseFee() domain.BaseFee {
$$$
}'
Length of output: 99
Script:
#!/bin/bash
# Let's try a different approach to find implementations
# First, find all Go files that might contain repository implementations
fd "repository.*\.go$"
# Then search for struct types that might implement RouterRepository
rg -A 5 "type.*Repository.*struct"
# Also search for GetBaseFee implementations specifically
rg "func.*GetBaseFee.*domain\.BaseFee"
Length of output: 2323
domain/mocks/router_usecase_mock.go (1)
38-40: LGTM: BaseFee field addition
The new field correctly extends the mock to support base fee functionality, aligning with the PR's objective to return base fee information.
app/sidecar_query_server.go (2)
28-28: LGTM: Import for base fee plugin
The addition of the basefee plugin import aligns with the PR objective of returning base fee in the /quote endpoint.
221-227: Verify the impact of switching to MsgSimulator
The change from GasCalculator to MsgSimulator is significant. While this aligns with the PR objective of returning base fee, we should verify that:
- Existing gas calculation functionality remains intact
- Quote simulation still works as expected
Let's verify the MsgSimulator implementation and its usage:
✅ Verification successful
Let me gather more information about the implementation and tests to verify the functionality.
Let me check one more aspect to verify the gas calculation implementation details.
Based on the gathered information, I can now provide the final response:
✅ MsgSimulator change maintains existing functionality
The switch from GasCalculator to MsgSimulator is a safe refactoring that maintains the existing gas calculation functionality:
- MsgSimulator interface properly encapsulates all required gas calculation capabilities including
SimulateMsgsandBuildTx - The
CalculateGasfunction is still used internally, ensuring core gas calculation logic remains unchanged - The implementation is well-tested as evidenced by test files in multiple packages
- Quote simulator integration is properly maintained through dependency injection
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify MsgSimulator implementation and its usage
# Check MsgSimulator implementation
echo "Checking MsgSimulator implementation..."
ast-grep --pattern 'type MsgSimulator struct {
$$$
}'
# Check if there are any tests for MsgSimulator
echo "Checking for MsgSimulator tests..."
fd -e go -x rg -l "TestMsgSimulator"
# Check for any direct usage of MsgSimulator
echo "Checking MsgSimulator usage..."
rg "NewMsgSimulator|MsgSimulator"
Length of output: 3969
Script:
#!/bin/bash
# Check MsgSimulator implementation details
echo "Checking MsgSimulator implementation details..."
rg -A 20 "type MsgSimulator interface" domain/cosmos/tx/
# Check quote simulator tests
echo "Checking quote simulator tests..."
rg -A 20 "func TestQuoteSimulator" quotesimulator/
# Check gas calculation implementation
echo "Checking gas calculation implementation..."
rg "func CalculateGas" domain/cosmos/tx/
Length of output: 1996
Script:
#!/bin/bash
# Check CalculateGas implementation and its usage
echo "Checking CalculateGas implementation..."
rg -A 10 "func CalculateGas" domain/cosmos/tx/
# Check gasCalculator struct implementation
echo "Checking gasCalculator struct..."
ast-grep --pattern 'type gasCalculator struct {
$$$
}'
# Check MsgSimulator struct implementation
echo "Checking concrete MsgSimulator implementation..."
rg -A 10 "type msgSimulator struct" domain/cosmos/tx/
Length of output: 1057
router/usecase/router_usecase.go (2)
888-891: LGTM: Clean implementation of GetBaseFee method.
The implementation correctly delegates to the router repository and aligns with the PR objective of returning base fee regardless of simulation success.
888-891: Verify interface implementation completeness.
Let's verify that all implementations of mvc.RouterUsecase interface include the GetBaseFee method and that it's properly integrated with the quote endpoint logic.
✅ Verification successful
Let me gather more information about the interface implementation by searching for the interface definition and its implementations.
Let me search for the SimpleRouterUsecase interface since RouterUsecase embeds it, and also search for any other implementations of GetBaseFee.
Implementation is correctly integrated
The GetBaseFee method is properly integrated in the codebase:
- It's correctly defined in the
RouterUsecaseinterface indomain/mvc/router.go - The implementation in
routerUseCaseImplcorrectly delegates to the repository layer - The repository layer (
routerRepo) has proper thread-safe implementation with mutex protection - The method is being used in the quote endpoint logic as seen in
router/delivery/http/router_handler.goanddomain/cosmos/tx/msg_simulator.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify interface implementation and usage
# Test 1: Check for interface implementations
echo "Checking for RouterUsecase interface implementations..."
ast-grep --pattern 'type $_ struct {
$$$
}
var _ mvc.RouterUsecase = $_'
# Test 2: Check for GetBaseFee usage in quote endpoint
echo "Checking for GetBaseFee usage in quote endpoint..."
rg -A 5 'func.*quote' --glob '**/*.go'
Length of output: 39177
Script:
#!/bin/bash
# Check for RouterUsecase interface definition
echo "Checking RouterUsecase interface definition..."
ast-grep --pattern 'type RouterUsecase interface {
$$$
}'
# Check for GetBaseFee method implementations
echo "Checking GetBaseFee implementations..."
ast-grep --pattern 'func ($VAR $_) GetBaseFee() domain.BaseFee {'
Length of output: 5890
Script:
#!/bin/bash
# Check SimpleRouterUsecase interface definition
echo "Checking SimpleRouterUsecase interface definition..."
ast-grep --pattern 'type SimpleRouterUsecase interface {
$$$
}'
# Search for any GetBaseFee implementations with ripgrep for broader coverage
echo "Searching for GetBaseFee implementations..."
rg "GetBaseFee" --type go -A 3
Length of output: 3241
domain/cosmos/tx/msg_simulator.go (1)
95-97: Consider refining error handling in BuildTx
Checking if priceInfo.Err != "" to determine if an error occurred is not idiomatic Go. It's preferable to have PriceMsgs return an error alongside QuotePriceInfo to handle errors more effectively.
| baseFee, err := tx.CalculateFeePrice(ctx, p.txfeesClient) | ||
| if err != nil { | ||
| p.logger.Error("failed to calculate fee price", zap.Error(err)) | ||
| } | ||
|
|
||
| p.routerRepository.SetBaseFee(baseFee) | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Error handling needs improvement, but thread safety is properly implemented.
The implementation of SetBaseFee in memoryRouterRepository is thread-safe using a mutex (baseFeeMx), so that concern is addressed. However, the error handling issues remain valid:
- Swallowing the error from
CalculateFeePriceand proceeding to update the repository could lead to inconsistent state - Returning nil masks critical failures from the caller
Suggested implementation:
func (p *baseFeeEndBlockUpdatePlugin) ProcessEndBlock(ctx context.Context, blockHeight uint64, metadata domain.BlockPoolMetadata) error {
baseFee, err := tx.CalculateFeePrice(ctx, p.txfeesClient)
if err != nil {
p.logger.Error("failed to calculate fee price", zap.Error(err))
return err
}
p.routerRepository.SetBaseFee(baseFee)
return nil
}🔗 Analysis chain
Review error handling strategy.
The current implementation:
- Logs but swallows errors from
CalculateFeePrice - Always returns nil, potentially masking critical failures
- Updates repository with potentially zero/invalid base fee on error
This behavior needs review:
- If base fee calculation fails, should we really proceed with updating the repository?
- Should we propagate errors to the caller for better error handling?
Also, consider documenting thread safety assumptions for SetBaseFee.
Let's verify the thread safety of RouterRepository:
Suggested implementation:
func (p *baseFeeEndBlockUpdatePlugin) ProcessEndBlock(ctx context.Context, blockHeight uint64, metadata domain.BlockPoolMetadata) error {
baseFee, err := tx.CalculateFeePrice(ctx, p.txfeesClient)
if err != nil {
p.logger.Error("failed to calculate fee price", zap.Error(err))
+ return err
}
p.routerRepository.SetBaseFee(baseFee)
return nil
}Let me check the implementation of RouterRepository to verify thread safety concerns.
Let me try a different approach to check the implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if RouterRepository interface or implementations mention thread safety
rg -i "thread.*safe|concurrent|sync" --type go
Length of output: 17950
Script:
#!/bin/bash
# Check RouterRepository implementation
ast-grep --pattern 'type memoryRouterRepository struct {
$$$
}'
# Check SetBaseFee implementation
ast-grep --pattern 'func $_($_ *memoryRouterRepository) SetBaseFee($_) {
$$$
}'
Length of output: 151
Script:
#!/bin/bash
# Search for SetBaseFee implementation and surrounding context
rg -A 10 "func.*SetBaseFee" --type go
# Check RouterRepository interface definition
rg -A 10 "type RouterRepository interface" --type go
Length of output: 2914
There was a problem hiding this comment.
This seems to be a valid suggestion.
There was a problem hiding this comment.
@cryptomatictrader, glad to hear that you find the suggestion helpful. Let me know if you need any further assistance.
There was a problem hiding this comment.
+1, I think we could make one step further by letting client of ProcessEndBlock logging message while actual implementation of ProcessEndBlock would be responsible only for constructing and adding required context to the error:
func (p *baseFeeEndBlockUpdatePlugin) ProcessEndBlock(ctx context.Context, blockHeight uint64, metadata domain.BlockPoolMetadata) error {
baseFee, err := tx.CalculateFeePrice(ctx, p.txfeesClient)
if err != nil {
return errors.New("failed to calculate fee price %w", err)
}
p.routerRepository.SetBaseFee(baseFee)
return nil
}There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
router/usecase/routertesting/parsing/quote_amount_in_response_base_fee.json
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
p0mvn
changed the title
| baseFee, err := tx.CalculateFeePrice(ctx, p.txfeesClient) | ||
| if err != nil { | ||
| p.logger.Error("failed to calculate fee price", zap.Error(err)) | ||
| } | ||
|
|
||
| p.routerRepository.SetBaseFee(baseFee) | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
This seems to be a valid suggestion.
quotesimulator/quote_simulator.go
Outdated
| gasAdjusted, feeCoin, err := q.msgSimulator.PriceMsgs(ctx, q.txFeesClient, q.encodingConfig.TxConfig, baseAccount, q.chainID, swapMsg) | ||
| if err != nil { | ||
| return 0, sdk.Coin{}, err | ||
| return domain.QuotePriceInfo{Err: err.Error()} |
There was a problem hiding this comment.
Will this return an empty Error struct/string, causing the caller consider it is a valid quote price data to be used? Suggest to have some meaning error message in it.
There was a problem hiding this comment.
I don't understand the concern, This returns a struct with an error embedded that the caller can check
| calculateGas: calculateGas, | ||
| clientCtx: clientCtx, | ||
| calculateGas: calculateGas, | ||
| memoryRouterRepository: memoryRouterRepository, |
There was a problem hiding this comment.
NOTE: See first this comment.
Can we instead of pulling in routerrepo.RouterRepository define more focused interface, for example:
type FeeSource interface {
GetBaseFee() domain.BaseFee
}| import "github.com/osmosis-labs/osmosis/osmomath" | ||
|
|
||
| // BaseFee holds the denom and current base fee | ||
| type BaseFee struct { |
There was a problem hiding this comment.
Do we really want to have new abstraction? I think sdk.Coin already would decribe it quite well:
fee := sdk.Coin{Denom: "osmo", Amount: 999}We can have type alias, but I still personally find it redundant:
Fee = skd.CoinThere was a problem hiding this comment.
- This has a
Dec - A separate abstraction is introduced for flexibility of extension (i.e if we need to add another piece of information, we would not need to break 10 different APIs to propagate this through the call stack. Instead, modify a single struct). That was a primary reason that this separate struct was introduced
| if err != nil { | ||
| return nil, err | ||
| priceInfo := c.PriceMsgs(ctx, encodingConfig.TxConfig, account, chainID, msg...) | ||
| if priceInfo.Err != "" { |
There was a problem hiding this comment.
While PriceInfo abstraction might have some benefits, I am not sure why we changing error type to string, it seems like this abstraction encapsulates some client logic that requires this specific format? I think txGasCalulator should be client indepdendent abstraction.
There was a problem hiding this comment.
It was changed to string so that we can return this to the client as a JSON response.
Duplicating err error when a string error message is already present feels unnecessary.
Happy for this to be iterated in a follow-up if you're feeling strongly
domain/cosmos/tx/msg_simulator.go
Outdated
| @@ -145,23 +147,33 @@ func (c *txGasCalulator) SimulateMsgs(encodingConfig cosmosclient.TxConfig, acco | |||
| } | |||
|
|
|||
| // PriceMsgs implements MsgSimulator. | |||
| func (c *txGasCalulator) PriceMsgs(ctx context.Context, txfeesClient txfeestypes.QueryClient, encodingConfig cosmosclient.TxConfig, account *authtypes.BaseAccount, chainID string, msg ...sdk.Msg) (uint64, sdk.Coin, error) { | |||
| func (c *txGasCalulator) PriceMsgs(ctx context.Context, encodingConfig cosmosclient.TxConfig, account *authtypes.BaseAccount, chainID string, msg ...sdk.Msg) domain.QuotePriceInfo { | |||
There was a problem hiding this comment.
txGasCalculator is used in other contexts such as claimbot too, should we rename domain.QuotePriceInfo to domain.TxFeeInfo, for example?
| @@ -145,23 +147,33 @@ func (c *txGasCalulator) SimulateMsgs(encodingConfig cosmosclient.TxConfig, acco | |||
| } | |||
|
|
|||
| // PriceMsgs implements MsgSimulator. | |||
| func (c *txGasCalulator) PriceMsgs(ctx context.Context, txfeesClient txfeestypes.QueryClient, encodingConfig cosmosclient.TxConfig, account *authtypes.BaseAccount, chainID string, msg ...sdk.Msg) (uint64, sdk.Coin, error) { | |||
| func (c *txGasCalulator) PriceMsgs(ctx context.Context, encodingConfig cosmosclient.TxConfig, account *authtypes.BaseAccount, chainID string, msg ...sdk.Msg) domain.QuotePriceInfo { | |||
| baseFee := c.memoryRouterRepository.GetBaseFee() | |||
There was a problem hiding this comment.
Do we want to keep backwards compatibility with previous implementation where we are able to compute fee by making request to chain instead of using pre-computed fee value? For example, is there any possible scenario where we might want to recompute fee for each simulation?
There was a problem hiding this comment.
The base fee can only be updated after processing transactions. As a result, refetching it once per-block is appropriate
|
|
||
| logger: logger, | ||
| } | ||
| } | ||
|
|
||
| // GetBaseFee implements RouterRepository. | ||
| func (r *routerRepo) GetBaseFee() domain.BaseFee { |
There was a problem hiding this comment.
Router has already a lot of responsibilities, introducing new one has resulted in ripple effect in the code base, notice how many changes in different places was required to make to implement this base fee idea. Additionally, I think router is not good candidate for maintaining baseFee, because many other parts in the system will use/benefit of this baseFee and each part now will be required to pull RouterRepository.
I would recommend creating instead fees ( or similar package ) and move relevant fee logic there, for example:
// GetBaseFee ...
func GetBaseFee() sdk.Coin {
r.baseFeeMx.RLock()
defer r.baseFeeMx.RUnlock()
return r.baseFee
}
// SetBaseFee ...
func SetBaseFee(baseFee sdk.Coin) {
r.baseFeeMx.Lock()
defer r.baseFeeMx.Unlock()
r.baseFee = baseFee
}Using this approach clients would not even need to have any dependency and can call fees.GetBaseFee().
There was a problem hiding this comment.
I created a separate interface that the router repository extends per your earlier comment so that tx gas calculator depends on a lighter interface.
IMO it is premature to start fragmenting the repository abstractions.
The base fee is used only by the router functionality. As a result, I chose to place it into the router repository
|
|
Applied recommendations and explained wherever there was a justification to not pursue a suggestion. Thanks for reviews |
…#550) * refactor: return base fee in /quote regardless of simulation success. * separate base fee from simulation. Pre-compute during ingest * updates * updates * Update router/repository/memory_router_repository.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix * separate interface * rename --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> (cherry picked from commit fcbf7b6) # Conflicts: # CHANGELOG.md # app/sidecar_query_server.go # docs/docs.go # docs/swagger.json # docs/swagger.yaml # domain/cosmos/tx/msg_simulator.go # domain/cosmos/tx/msg_simulator_test.go # domain/cosmos/tx/tx.go # domain/mocks/msg_simulator_mock.go # domain/mocks/quote_mock.go # domain/mocks/quote_simulator_mock.go # domain/quote_simulator.go # domain/router.go # ingest/usecase/plugins/orderbook/claimbot/config.go # ingest/usecase/plugins/orderbook/claimbot/export_test.go # ingest/usecase/plugins/orderbook/claimbot/plugin.go # ingest/usecase/plugins/orderbook/claimbot/tx.go # ingest/usecase/plugins/orderbook/claimbot/tx_test.go # quotesimulator/quote_simulator.go # quotesimulator/quote_simulator_test.go # router/delivery/http/router_handler.go # router/delivery/http/router_handler_test.go # router/types/get_quote_request.go # router/usecase/quote_out_given_in.go # router/usecase/routertesting/parsing/quote_amount_in_response_simulated.json
…#550) * refactor: return base fee in /quote regardless of simulation success. * separate base fee from simulation. Pre-compute during ingest * updates * updates * Update router/repository/memory_router_repository.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix * separate interface * rename --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…#550) (#559) * refactor: return base fee in /quote regardless of simulation success. * separate base fee from simulation. Pre-compute during ingest * updates * updates * Update router/repository/memory_router_repository.go * fix * separate interface * rename --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…#550) * refactor: return base fee in /quote regardless of simulation success. * separate base fee from simulation. Pre-compute during ingest * updates * updates * Update router/repository/memory_router_repository.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix * separate interface * rename --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Several changes:
Ultimately, this will allow us to compute fees more precisely in a more performant way in the Polaris SQS integration while having gas estimates hardcoded.
The next big improvement would be to implement swap simulations without requiring bank sends. Documented the proposal in this chain task
Testing
Summary by CodeRabbit
Release Notes
New Features
appendBaseFeein the/router/quoteendpoint to include base fees in the quote response./quoteendpoint regardless of simulation success./quoteendpoint to support simulations for "out given in" single routes.TxFeeInfoto encapsulate transaction fee information, including base fee and error messages.Bug Fixes
Documentation
appendBaseFeeparameter and changes in response structures.Chores