refactor: remove batch bridge fee logic #728
Conversation
WalkthroughThis pull request includes extensive modifications across multiple files, primarily focusing on refining the cross-chain functionality and related components. Key changes involve the removal of deprecated functions and events, adjustments to message types, and updates to the API specifications. The linting process in the Makefile has also been refined. Additionally, several test files and methods related to removed functionalities have been deleted or modified to reflect these changes. Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Outside diff range and nitpick comments (22)
docs/swagger-ui/swagger.yaml (5)
5360-5418: LGTM! Consider enhancing the description.The restructuring of the
/fx/crosschain/v1/batch/outgoing_txendpoint and its response schema looks good. The change from returning multiple batches to a single batch aligns well with the endpoint's singular name.Consider updating the description of the
OutgoingTxBatchobject (line 5419-5420) to reflect that it represents a single batch rather than multiple batches. For example:title: >- OutgoingTxBatch represents a single batch of transactions going from gravity to ETH
5620-5695: LGTM! Consider clarifying the description.The addition of the
/fx/crosschain/v1/batch/outgoing_txsendpoint is a good complement to the single batch endpoint. It provides a way to retrieve multiple batches and includes useful query parameters for filtering.Consider adding a description to the endpoint to clarify its purpose and how it differs from the single batch endpoint. For example, you could add:
description: Retrieves multiple outgoing transaction batches, optionally filtered by token contract and nonce.Also, consider updating the description of the
OutgoingTxBatchobject (line 5693-5695) to be consistent with the suggested change in the previous comment.
5897-5942: LGTM! Consider adding a description and query parameters.The addition of the
/fx/crosschain/v1/bridge_call_by_nonceendpoint is well-structured and aligns with the PR objective. Thebridge_callobject in the response schema is comprehensive and includes all necessary information.Consider the following improvements:
- Add a description to the endpoint to clarify its purpose:
description: Retrieves a specific bridge call by its nonce.
- Add a query parameter for the nonce, as it's likely required to identify the specific bridge call:
parameters: - name: nonce in: query required: true type: string format: uint64 description: The nonce of the bridge call to retrieve.These additions would improve the clarity and usability of the API documentation.
11885-11896: LGTM! Consider enhancing the documentation.The addition of the
/fx/crosschain/v1/projected_batch_timeoutendpoint is straightforward and aligns with the PR objective. The simple response structure with just thetimeout_heightis appropriate for its purpose.Consider the following improvements to enhance the API documentation:
- Add a description to the endpoint to clarify its purpose:
description: Retrieves the projected timeout height for the next batch.
- Add query parameters if any are needed to calculate the projected timeout. For example:
parameters: - name: token_contract in: query required: false type: string description: The token contract address to calculate the timeout for (optional).
- Add a description to the
timeout_heightproperty in the response:timeout_height: type: string format: uint64 description: The projected block height at which the next batch will timeout.These additions would provide more context and improve the usability of the API.
44050-44102: LGTM! Consider adding property descriptions.The addition of the
fx.gravity.crosschain.v1.QueryOutgoingTxBatchResponsedefinition is consistent with the changes made to the/fx/crosschain/v1/batch/outgoing_txendpoint. The structure is comprehensive and includes all necessary properties for an outgoing transaction batch.To improve the documentation, consider adding descriptions to the main properties of the
batchobject. For example:batch: type: object description: Represents a single batch of outgoing transactions. properties: batch_nonce: type: string format: uint64 description: Unique identifier for the batch. batch_timeout: type: string format: uint64 description: Block height at which this batch times out. transactions: type: array description: List of transactions included in this batch. items: # ... (existing properties) token_contract: type: string description: The token contract address for this batch. block: type: string format: uint64 description: Block height at which this batch was created. feeReceive: type: string description: Address that receives the fees for this batch.Adding these descriptions would provide more context and improve the overall documentation quality.
proto/fx/gravity/crosschain/v1/legacy.proto (2)
Line range hint
1-132: Summary of changes in legacy.protoThis file has undergone significant changes as part of the effort to remove batch bridge fee logic:
- Multiple messages have been deprecated, including
MsgSendToExternal,MsgRequestBatch, and others.- Field names have been updated for consistency, notably changing
outgoing_tx_idtobatch_nonce.- Deprecation notices have been updated to provide clearer guidance on alternative approaches.
These changes suggest a substantial shift in the system's architecture or design. Ensure that all deprecated messages are properly handled and that any code depending on the changed field names is updated accordingly.
Consider creating a migration guide or updating existing documentation to reflect these changes and guide users in transitioning to the new architecture.
Incomplete Deprecation of
MsgRequestBatchResponseThe
MsgRequestBatchResponsemessage is still referenced in the following locations:
x/crosschain/types/legacy.pb.goproto/fx/gravity/crosschain/v1/legacy.protoPlease ensure that all usages of
MsgRequestBatchResponseare removed or updated to complete the deprecation process.🔗 Analysis chain
Line range hint
110-110: Note deprecation of MsgRequestBatchResponseThe
MsgRequestBatchResponsemessage has been explicitly marked as deprecated with the comment "do not use". This change is consistent with the overall trend of deprecating certain messages in this file. The field name 'batch_nonce' remains unchanged, maintaining consistency with the change made toMsgSendToExternalResponse.To ensure proper handling of this deprecation, please run the following script to check for any remaining uses of
MsgRequestBatchResponse:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to MsgRequestBatchResponse in the codebase rg --type-add 'proto:*.proto' --type proto --type go 'MsgRequestBatchResponse'Length of output: 2537
tests/precompile_test.go (1)
Line range hint
1-132: Overall impact assessment and suggested next steps.The changes in this file consistently remove cross-chain transfer test cases across multiple methods, aligning with the PR objective of removing batch bridge fee logic. While this is likely necessary due to architectural changes, it raises concerns about maintaining comprehensive test coverage.
To ensure the quality and reliability of the codebase:
- Conduct a comprehensive review of the test suite to identify any gaps in coverage resulting from these changes.
- Develop new test cases that cover the updated functionality, ensuring that all critical paths are still being tested.
- Consider implementing integration or end-to-end tests that verify the system's behavior in the absence of the removed batch bridge fee logic.
- Update documentation to reflect the changes in the testing approach and any new test cases added.
Given the significant changes to the test suite, it may be beneficial to revisit the overall testing strategy for cross-chain functionality. Consider implementing a more modular testing approach that can easily adapt to future changes in the cross-chain architecture.
x/crosschain/precompile/keeper.go (2)
136-138: Approve the change to batch processing.The change from
AddToOutgoingPooltoBuildOutgoingTxBatchappears to be an improvement in the transaction handling mechanism. The new method name is more descriptive of its functionality, and the use ofbatchNonceas the return value is more appropriate.Consider updating any related documentation or comments to reflect this change in batch processing approach.
136-141: Summary of changes and potential impact.The modifications in this file appear to be part of a larger refactoring effort to improve the batch processing of outgoing transactions in the cross-chain functionality. The changes from
AddToOutgoingPooltoBuildOutgoingTxBatchand the consistent use ofbatchNoncesuggest a more structured approach to handling outgoing transfers.While these changes improve code clarity and potentially efficiency, they may have broader implications:
- Performance: The new batch building approach might affect the timing and grouping of transactions.
- Consistency: Ensure that all related components (e.g., event emissions, logging) are updated to reflect the new batch-oriented approach.
- API Changes: If
txIDwas exposed in any public APIs, those might need to be updated or versioned.Consider reviewing the entire cross-chain module to ensure these changes are consistently applied and that they align with the overall architecture goals. It may be beneficial to update the module's documentation to reflect this new approach to transaction batching.
Makefile (1)
145-145: LGTM! Consider making the linter check more flexible.The reduction in allowed 'nolint' or '#nosec' occurrences from 31 to 29 reflects an improvement in code quality. This change aligns well with the PR objective of refactoring and removing deprecated code.
To make this check more maintainable in the future, consider using a range instead of an exact number. This would allow for small fluctuations without requiring frequent updates to the Makefile. For example:
- @if [ $$(find . -name '*.go' -type f | xargs grep 'nolint\|#nosec' | wc -l) -ne 29 ]; then \ + @if [ $$(find . -name '*.go' -type f | xargs grep 'nolint\|#nosec' | wc -l) -lt 25 ] || [ $$(find . -name '*.go' -type f | xargs grep 'nolint\|#nosec' | wc -l) -gt 35 ]; then \This change would allow for a range of 25-35 occurrences, providing some flexibility while still catching significant increases in linter suppressions.
proto/fx/gravity/crosschain/v1/tx.proto (4)
29-30: Approved: ConfirmBatch deprecation consistent with API simplification.The deprecation of
ConfirmBatchin favor ofConfirmis consistent with the previous change and supports the goal of simplifying the API.Consider updating any related documentation or guides that may reference
ConfirmBatchto reflect this change and direct users to the newConfirmmethod.
31-33: Approved: BridgeCallConfirm deprecation completes API simplification.The deprecation of
BridgeCallConfirmin favor ofConfirmcompletes the pattern of simplifying the confirmation API. This change is in line with the overall refactoring goal.To ensure a smooth transition:
- Update all client code that uses these deprecated methods to use the new
Confirmmethod.- Consider adding a migration guide for users of the API.
- Plan for the eventual removal of these deprecated methods in a future release.
37-37: Approved: BridgeCall method added as replacement for SendToExternal.The addition of the
BridgeCallmethod is a positive step, providing a replacement for the deprecatedSendToExternalmethod.To ensure smooth adoption of this new method:
- Please add comprehensive documentation for
BridgeCall, including its parameters, return value, and expected behavior.- Consider providing a code example demonstrating how to use
BridgeCallin place ofSendToExternal.- Update any relevant API documentation or developer guides to reflect this new method.
26-37: Summary: Significant API changes align with refactoring goals.The changes in this file represent a significant refactoring of the cross-chain API:
- Deprecation of
OracleSetConfirm,ConfirmBatch, andBridgeCallConfirmin favor of a unifiedConfirmmethod.- Deprecation of
SendToExternalin favor of a newBridgeCallprecompile.- Addition of a new
BridgeCallRPC method.These changes align well with the PR objective of removing batch bridge fee logic and simplifying the API. However, they will require careful migration of existing client code.
To ensure a smooth transition:
- Create a comprehensive migration guide for API users.
- Update all internal usages of the deprecated methods.
- Consider adding temporary backwards compatibility measures if this is a widely-used public API.
- Plan for a future release to remove the deprecated methods entirely.
- Update all relevant documentation, including API docs, developer guides, and examples.
x/tron/keeper/grpc_query_test.go (1)
Line range hint
13-130: LGTM! Consider enhancing error handling in test cases.The refactoring from
TestKeeper_BatchRequestByNoncetoTestKeeper_OutgoingTxBatchimproves clarity and aligns well with the underlying functionality being tested. The changes to request and response types, as well as the method call updates, are consistent and appropriate.Consider enhancing the error handling in the test cases that expect failures (lines 125-131). Instead of just checking for the presence of an error, you could verify the specific error type or message to ensure the correct error is being returned. This would make the tests more robust and informative. For example:
if testCase.expPass { suite.Require().NoError(err) suite.Require().Equal(response.Batch, res.Batch) } else { suite.Require().Error(err) // Add specific error checks here, e.g.: // suite.Require().Contains(err.Error(), "expected error message") // or // suite.Require().IsType(&expectedErrorType{}, err) }This enhancement would provide more confidence that the error handling in the actual implementation is working as expected.
x/crosschain/keeper/grpc_query_router.go (1)
Line range hint
1-1: Summary of changes and potential impact.The changes in this file, particularly the refactoring of
BatchRequestByNoncetoOutgoingTxBatchand the removal ofBatchFees, are consistent with the PR objective of removing batch bridge fee logic. These modifications improve code clarity but may have broader implications:
- The renaming of methods may require updates in other parts of the codebase that use these functions.
- The removal of batch fee functionality could affect how fees are handled or queried throughout the system.
- There may be a need to update documentation, API specifications, and potentially client-side code that interacts with these endpoints.
Consider the following to ensure a smooth transition:
- Update all relevant documentation and API specifications.
- Review and update any client-side code that may be affected by these changes.
- Consider adding a deprecation notice or temporary backwards compatibility for any external systems that might be using the old endpoints.
- Ensure that the removal of batch fees doesn't create any inconsistencies in the overall fee handling logic of the system.
x/crosschain/client/cli/query.go (1)
Line range hint
511-538: LGTM! Consider adding error handling for empty results.The
CmdOutgoingTxBatchfunction is well-implemented and correctly replaces the previousCmdBatchRequestByNonce. It properly validates inputs and uses the newQueryOutgoingTxBatchRequesttype.Consider adding a check for empty results before printing the protocol buffer. This could improve the user experience by providing a clear message when no batch is found:
if err != nil { return err } +if res.Batch == nil { + return fmt.Errorf("no outgoing tx batch found for token contract %s and nonce %d", tokenContract, nonce) +} return clientCtx.PrintProto(res.Batch)x/crosschain/types/key.go (1)
Line range hint
220-223: Add error handling toParseOutgoingBridgeCallNonceto ensure correct parsing.The function
ParseOutgoingBridgeCallNonceassumes that the providedkeycontains the expected prefixes. If thekeydoes not have theOutgoingBridgeCallAddressAndNonceKeyprefix or theaddressprefix,bytes.TrimPrefixwill not alter the slice, potentially leading to incorrect nonce values or panics. To enhance robustness, consider adding checks to verify the presence of these prefixes and handle cases where they are missing.Apply this diff to incorporate error handling:
+import "fmt" -func ParseOutgoingBridgeCallNonce(key []byte, address string) (nonce uint64) { +func ParseOutgoingBridgeCallNonce(key []byte, address string) (nonce uint64, err error) { + if !bytes.HasPrefix(key, OutgoingBridgeCallAddressAndNonceKey) { + return 0, fmt.Errorf("key does not have expected OutgoingBridgeCallAddressAndNonceKey prefix") + } addrNonce := bytes.TrimPrefix(key, OutgoingBridgeCallAddressAndNonceKey) + if !bytes.HasPrefix(addrNonce, []byte(address)) { + return 0, fmt.Errorf("key does not have expected address prefix") + } nonceBytes := bytes.TrimPrefix(addrNonce, []byte(address)) - return sdk.BigEndianToUint64(nonceBytes) + return sdk.BigEndianToUint64(nonceBytes), nil }x/crosschain/keeper/grpc_query.go (1)
Line range hint
138-149: Enhance error messages for better user understandingThe error messages returned in case of invalid input could be more descriptive. Instead of generic terms like "nonce" or "token contract address", provide clearer messages that specify the issue. This will improve usability by helping users understand what went wrong.
Apply this diff to improve the error messages:
func (k QueryServer) OutgoingTxBatch(c context.Context, req *types.QueryOutgoingTxBatchRequest) (*types.QueryOutgoingTxBatchResponse, error) { if err := types.ValidateExternalAddr(req.ChainName, req.GetTokenContract()); err != nil { - return nil, status.Error(codes.InvalidArgument, "token contract address") + return nil, status.Errorf(codes.InvalidArgument, "invalid token contract address: %v", err) } if req.GetNonce() <= 0 { - return nil, status.Error(codes.InvalidArgument, "nonce") + return nil, status.Error(codes.InvalidArgument, "invalid nonce: must be greater than 0") } foundBatch := k.GetOutgoingTxBatch(sdk.UnwrapSDKContext(c), req.TokenContract, req.Nonce) if foundBatch == nil { - return nil, status.Error(codes.NotFound, "tx batch") + return nil, status.Errorf(codes.NotFound, "outgoing transaction batch not found for token contract %s and nonce %d", req.TokenContract, req.Nonce) } return &types.QueryOutgoingTxBatchResponse{Batch: foundBatch}, nil }x/crosschain/keeper/grpc_query_v1_test.go (2)
Line range hint
791-799: Ensure test case names are uniqueDuplicate test case names can cause confusion when running tests and interpreting results. The test case named
"query token contract error"is used more than once. Please update the test case names to be unique and descriptive.
Line range hint
804-812: Rename test case to reflect the error being testedThe test case name is
"query token contract error", but it tests for an invalid nonce error. To improve clarity, consider renaming the test case to accurately reflect the scenario being tested.Apply this diff to fix the test case name:
- name: "query token contract error", + name: "query nonce error",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (4)
x/crosschain/types/legacy.pb.gois excluded by!**/*.pb.gox/crosschain/types/query.pb.gois excluded by!**/*.pb.gox/crosschain/types/query.pb.gw.gois excluded by!**/*.pb.gw.gox/crosschain/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (52)
- Makefile (1 hunks)
- app/app_test.go (1 hunks)
- app/encoding_test.go (1 hunks)
- app/interface_registry.json (0 hunks)
- contract/ICrossChain.go (1 hunks)
- docs/swagger-ui/swagger.yaml (5 hunks)
- docs/swagger_test.go (1 hunks)
- proto/fx/gravity/crosschain/v1/legacy.proto (2 hunks)
- proto/fx/gravity/crosschain/v1/query.proto (2 hunks)
- proto/fx/gravity/crosschain/v1/tx.proto (1 hunks)
- solidity/contracts/bridge/ICrossChain.sol (0 hunks)
- solidity/contracts/test/CrossChainTest.sol (0 hunks)
- tests/contract/CrossChainTest.go (1 hunks)
- tests/crosschain_suite.go (0 hunks)
- tests/crosschain_test.go (0 hunks)
- tests/integration_test.go (0 hunks)
- tests/precompile_suite.go (0 hunks)
- tests/precompile_test.go (5 hunks)
- x/crosschain/client/cli/query.go (3 hunks)
- x/crosschain/client/cli/tx.go (2 hunks)
- x/crosschain/keeper/abci.go (1 hunks)
- x/crosschain/keeper/batch_fee.go (0 hunks)
- x/crosschain/keeper/batch_test.go (0 hunks)
- x/crosschain/keeper/bridge_call_out.go (0 hunks)
- x/crosschain/keeper/bridge_call_out_test.go (0 hunks)
- x/crosschain/keeper/bridge_token.go (0 hunks)
- x/crosschain/keeper/genesis.go (0 hunks)
- x/crosschain/keeper/genesis_test.go (0 hunks)
- x/crosschain/keeper/grpc_query.go (2 hunks)
- x/crosschain/keeper/grpc_query_router.go (1 hunks)
- x/crosschain/keeper/grpc_query_v1_test.go (5 hunks)
- x/crosschain/keeper/msg_server.go (2 hunks)
- x/crosschain/keeper/msg_server_router.go (0 hunks)
- x/crosschain/keeper/msg_server_test.go (0 hunks)
- x/crosschain/keeper/outgoing_pool.go (0 hunks)
- x/crosschain/keeper/outgoing_pool_test.go (0 hunks)
- x/crosschain/keeper/outgoing_tx.go (3 hunks)
- x/crosschain/precompile/cancel_send_to_external.go (0 hunks)
- x/crosschain/precompile/cancel_send_to_external_test.go (0 hunks)
- x/crosschain/precompile/contract.go (0 hunks)
- x/crosschain/precompile/crosschain_test.go (0 hunks)
- x/crosschain/precompile/expected_keepers.go (1 hunks)
- x/crosschain/precompile/increase_bridge_fee.go (0 hunks)
- x/crosschain/precompile/increase_bridgefee_test.go (0 hunks)
- x/crosschain/precompile/keeper.go (1 hunks)
- x/crosschain/types/events.go (0 hunks)
- x/crosschain/types/key.go (1 hunks)
- x/crosschain/types/msgs.go (0 hunks)
- x/crosschain/types/msgs_test.go (0 hunks)
- x/crosschain/types/params.go (0 hunks)
- x/crosschain/types/types.go (0 hunks)
- x/tron/keeper/grpc_query_test.go (8 hunks)
💤 Files with no reviewable changes (29)
- app/interface_registry.json
- solidity/contracts/bridge/ICrossChain.sol
- solidity/contracts/test/CrossChainTest.sol
- tests/crosschain_suite.go
- tests/crosschain_test.go
- tests/integration_test.go
- tests/precompile_suite.go
- x/crosschain/keeper/batch_fee.go
- x/crosschain/keeper/batch_test.go
- x/crosschain/keeper/bridge_call_out.go
- x/crosschain/keeper/bridge_call_out_test.go
- x/crosschain/keeper/bridge_token.go
- x/crosschain/keeper/genesis.go
- x/crosschain/keeper/genesis_test.go
- x/crosschain/keeper/msg_server_router.go
- x/crosschain/keeper/msg_server_test.go
- x/crosschain/keeper/outgoing_pool.go
- x/crosschain/keeper/outgoing_pool_test.go
- x/crosschain/precompile/cancel_send_to_external.go
- x/crosschain/precompile/cancel_send_to_external_test.go
- x/crosschain/precompile/contract.go
- x/crosschain/precompile/crosschain_test.go
- x/crosschain/precompile/increase_bridge_fee.go
- x/crosschain/precompile/increase_bridgefee_test.go
- x/crosschain/types/events.go
- x/crosschain/types/msgs.go
- x/crosschain/types/msgs_test.go
- x/crosschain/types/params.go
- x/crosschain/types/types.go
🧰 Additional context used
🔇 Additional comments (22)
docs/swagger-ui/swagger.yaml (1)
Line range hint
1-44102: Overall, excellent refactoring of the API specification.The changes made to the
swagger.yamlfile represent a significant improvement in the API design and align well with the PR objective of removing batch bridge fee logic. The new structure is more logical, consistent, and easier to understand. Key improvements include:
- Clearer separation of single and multiple batch retrieval endpoints.
- Introduction of a new
bridge_call_by_nonceendpoint to replace the previous batch fees endpoint.- Addition of a
projected_batch_timeoutendpoint for better batch management.- Consistent and well-structured response schemas across all endpoints.
The minor suggestions provided in the previous comments, if implemented, would further enhance the clarity and usability of the API documentation.
Great job on this refactoring effort!
app/encoding_test.go (1)
46-46: Verify the reduction in interface count and update documentation if needed.The change from 303 to 300 in the expected count of implementation interfaces aligns with the PR objective of removing batch bridge fee logic. This reduction indicates that some interfaces have been removed from the system.
To ensure consistency across the codebase:
- Verify that this change is reflected in other parts of the code where these interfaces might have been used.
- Update any relevant documentation that might reference the removed interfaces or the total count of interfaces.
Run the following script to check for any remaining references to the removed interfaces:
If you find any inconsistencies or outdated references, please update them accordingly.
x/crosschain/precompile/expected_keepers.go (1)
59-59: LGTM: Changes align with PR objectivesThe modification of the
CrosschainKeeperinterface, specifically the replacement ofAddToOutgoingPoolwithBuildOutgoingTxBatch, aligns with the PR objective of removing batch bridge fee logic. This change suggests a shift in how outgoing transactions are processed, potentially moving towards a more efficient batching mechanism.To ensure consistency across the codebase, please run the following verification scripts:
Please review the output of these scripts to ensure that:
- There are no remaining references to the removed methods.
- The new
BuildOutgoingTxBatchmethod is being used appropriately.- There are no lingering references to the old
AddToOutgoingPoolmethod.If any inconsistencies are found, they should be addressed to maintain the integrity of the codebase.
✅ Verification successful
**** The changes have been successfully verified. No lingering references to the removed methods were found, and the new
BuildOutgoingTxBatchmethod is utilized appropriately across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to removed methods and verify usage of the new method. # Test 1: Search for references to removed methods echo "Searching for references to removed methods..." rg --type go 'RemoveFromOutgoingPoolAndRefund|AddUnbatchedTxBridgeFee' # Test 2: Verify usage of the new BuildOutgoingTxBatch method echo "Verifying usage of BuildOutgoingTxBatch method..." rg --type go 'BuildOutgoingTxBatch' # Test 3: Check for any remaining references to AddToOutgoingPool echo "Checking for remaining references to AddToOutgoingPool..." rg --type go 'AddToOutgoingPool'Length of output: 1062
proto/fx/gravity/crosschain/v1/legacy.proto (2)
76-76: Update handling of MsgSendToExternalResponseThe field in
MsgSendToExternalResponsehas been changed fromoutgoing_tx_idtobatch_nonce. This change suggests a shift in how transactions are identified or grouped, possibly indicating that transactions are now processed in batches rather than individually.To ensure all references are updated, please run the following script:
#!/bin/bash # Search for references to outgoing_tx_id in the codebase rg --type-add 'proto:*.proto' --type proto --type go 'outgoing_tx_id'
63-63: Update references to MsgSendToExternalThe deprecation notice for
MsgSendToExternalhas been updated to recommend using "precompile BridgeCall" instead of "MsgBridgeCall". This change suggests a shift in the recommended approach for this functionality.To ensure consistency, please run the following script to check for any remaining references to
MsgBridgeCall:✅ Verification successful
No remaining references to
MsgBridgeCallfoundAll references to
MsgBridgeCallhave been successfully removed from the codebase, confirming that the deprecation ofMsgSendToExternalis properly handled. No further action is required.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to MsgBridgeCall in the codebase rg --type-add 'proto:*.proto' --type proto --type go 'MsgBridgeCall'Length of output: 322
tests/precompile_test.go (2)
28-30: Verify the removal of cross-chain transfer test cases.The commented out lines suggest that cross-chain transfer functionality is no longer being tested in this method. While this aligns with the PR objective of removing batch bridge fee logic, it's crucial to ensure that we're not leaving critical functionality untested.
Could you please confirm if these test cases are being replaced by alternative tests elsewhere? If not, consider adding appropriate test coverage for the remaining cross-chain transfer functionality.
✅ Verification successful
Cross-chain transfer test coverage confirmed elsewhere.
The search results indicate that cross-chain transfer related test cases are present in other test files, such as
tests/integration_test.goandtests/crosschain_test.go. This ensures that the removal of these test cases fromtests/precompile_test.godoes not leave critical functionality untested.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any new or modified test cases related to cross-chain transfers rg --type go -e "func.*CrossChain.*Test" --statsLength of output: 7480
44-46: Verify the purpose of the remaining code in PrecompileCrossChainTest.With the removal of the cross-chain transfer test cases, it appears that this method may no longer be fully testing its intended functionality.
Please review the remaining code in this method and consider the following:
- Does the method still serve a purpose without the cross-chain transfer tests?
- If so, should the method be renamed to reflect its new purpose?
- If not, should the entire method be removed?
proto/fx/gravity/crosschain/v1/tx.proto (1)
26-28: Approved: OracleSetConfirm deprecation aligns with refactoring goals.The deprecation of
OracleSetConfirmand the suggestion to useConfirminstead is a good step towards simplifying the API. This change aligns well with the PR objective to remove batch bridge fee logic.To ensure a smooth transition, please run the following script to check for any remaining usages of
OracleSetConfirmin the codebase:#!/bin/bash # Search for OracleSetConfirm usages rg --type proto --type go "OracleSetConfirm"x/crosschain/keeper/grpc_query_router.go (2)
94-100: Approve refactoring of batch querying functionality.The renaming from
BatchRequestByNoncetoOutgoingTxBatchimproves clarity and aligns with the removal ofBatchFees. The core logic remains intact, maintaining the pattern used throughout the file.To ensure this change doesn't introduce issues elsewhere in the codebase, please run the following script:
This will help identify any places where the old method name is still being used and confirm that the new method name is being used correctly.
✅ Verification successful
Approve refactoring of batch querying functionality.
The removal of
BatchRequestByNoncewas successful with no remaining references, and the newOutgoingTxBatchmethod is consistently used across the codebase. This refactoring enhances clarity and maintains existing functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to BatchRequestByNonce # Search for any remaining references to BatchRequestByNonce echo "Searching for remaining references to BatchRequestByNonce:" rg --type go "BatchRequestByNonce" # Search for usage of the new OutgoingTxBatch method echo "Searching for usage of the new OutgoingTxBatch method:" rg --type go "OutgoingTxBatch"Length of output: 36236
Line range hint
1-1: Verify impact of removing BatchFees method.The removal of the
BatchFeesmethod aligns with the PR objective to remove batch bridge fee logic. However, this change may have broader implications on the system.Please run the following script to identify any remaining references to
BatchFeesand ensure that all related functionality has been properly addressed:Additionally, please confirm that:
- Any code that previously relied on batch fee information has been updated or removed.
- The removal of batch fees doesn't introduce any inconsistencies in the fee handling logic.
- Documentation and API specifications have been updated to reflect this change.
x/crosschain/client/cli/query.go (3)
Line range hint
540-556: LGTM! Great addition for querying all outgoing tx batches.The
CmdOutgoingTxBatchesfunction is a well-implemented new command that allows querying all outgoing transaction batches. This is a valuable addition to the CLI, providing users with a comprehensive view of all pending outgoing transactions.
Line range hint
1-956: Removal of deprecated functions aligns with PR objectives.The removal of
CmdGetPendingSendToExternalandCmdGetBatchFeesfunctions is in line with the PR objective of removing batch bridge fee logic. This simplification of the CLI removes potentially outdated or unnecessary functionality, contributing to a cleaner and more focused codebase.
Line range hint
1-956: Overall changes improve and streamline the cross-chain CLI queries.The modifications in this file successfully refactor the outgoing transaction batch queries, aligning with the PR objectives. The addition of
CmdOutgoingTxBatchandCmdOutgoingTxBatchesprovides more comprehensive and flexible ways to query outgoing transaction batches. Simultaneously, the removal of deprecated functions related to batch fees (CmdGetPendingSendToExternalandCmdGetBatchFees) simplifies the module and removes outdated functionality.These changes contribute to a more focused and efficient CLI for cross-chain operations, improving the overall user experience and maintainability of the codebase.
x/crosschain/keeper/outgoing_tx.go (2)
56-70: Verify the correctness of telemetry metrics implementationTelemetry metrics have been added to track outgoing transactions. Please ensure that these metrics accurately capture the intended data and that all labels and values are correct. Additionally, consider error handling in case telemetry recording fails.
Run the following script to check for proper usage of telemetry functions:
#!/bin/bash # Description: Verify telemetry function calls for correctness. # Search for telemetry function usage in Go files rg --type go 'telemetry\.[A-Za-z]+\(' -A 2
15-15: Update all references toBuildOutgoingTxBatchwith the new signatureThe function
BuildOutgoingTxBatchhas changed its signature. Ensure that all calls to this function across the codebase are updated to match the new parameters to prevent any compilation or runtime errors.Run the following script to find all usages of
BuildOutgoingTxBatchand verify that they have been updated:x/crosschain/types/key.go (1)
207-209: Deprecation ofGetTokenToDenomKeyis properly indicated.The function
GetTokenToDenomKeyis correctly marked as deprecated. This clear documentation will help developers avoid using outdated functions.x/crosschain/client/cli/tx.go (1)
298-298: Consistent Method Calls on Renamed VariableEnsure that method calls on the renamed variable
outgoingTxBatchRespare correctly updated. In this line, the method chain should reflect the new variable name:proto/fx/gravity/crosschain/v1/query.proto (3)
65-65: Updated HTTP GET endpoint forOutgoingTxBatchesThe HTTP GET endpoint has been updated to
/fx/crosschain/v1/batch/outgoing_txs, which accurately reflects the plural form and improves clarity.
207-210: Definition ofQueryOutgoingTxBatchRequestis appropriateThe request message
QueryOutgoingTxBatchRequestincludes necessary fields (chain_name,token_contract, andnonce) to uniquely identify an outgoing transaction batch.
212-212: Definition ofQueryOutgoingTxBatchResponseis appropriateThe response message
QueryOutgoingTxBatchResponsecontains anOutgoingTxBatch, which correctly represents the batch details.x/crosschain/keeper/msg_server.go (1)
Line range hint
389-401:BridgeCallmethod implementation looks goodThe
BridgeCallfunction is correctly implemented and follows the required patterns for handling bridge calls, including error handling and event emission.tests/contract/CrossChainTest.go (1)
34-35: ABI and Binary Metadata Updated CorrectlyThe
ABIandBinfields inCrossChainTestMetaDatahave been appropriately updated to reflect the removal of theCancelSendToExternalandIncreaseBridgeFeefunctions. This ensures that the contract bindings are consistent with the updated contract interface.
Summary by CodeRabbit
New Features
Bug Fixes
Deprecations
CancelSendToExternalandIncreaseBridgeFee.Documentation