chore: move logic for getting packet data to its own function #6475
Conversation
DimitrisJim
requested review from
colin-axner,
AdityaSripal,
damiannolan and
chatton
as code owners
|
Warning Rate limit exceeded@DimitrisJim has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 47 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe recent changes in the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant sendTransfer
participant createPacketDataBytesFromVersion
participant PacketData
sendTransfer ->> createPacketDataBytesFromVersion: Request to create packet data bytes
createPacketDataBytesFromVersion ->> PacketData: Create packet data based on app version
PacketData -->> createPacketDataBytesFromVersion: Return packet data bytes
createPacketDataBytesFromVersion -->> sendTransfer: Return packet data bytes
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
| // Sanity check, tokens must always be of length 1 if using app version V1. | ||
| if len(tokens) != 1 { | ||
| panic(fmt.Errorf("length of tokens must be equal to 1 if using %s version", types.V1)) | ||
| } |
There was a problem hiding this comment.
not seeing this used elsewhere but since it has been moved to func that might be re-used I added the explicit check to avoid the nasty panic if empty/faulty logic if > 1.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- modules/apps/transfer/keeper/export_test.go (1 hunks)
- modules/apps/transfer/keeper/relay.go (2 hunks)
- modules/apps/transfer/keeper/relay_test.go (2 hunks)
Additional context used
Path-based instructions (3)
modules/apps/transfer/keeper/export_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/transfer/keeper/relay.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/transfer/keeper/relay_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
golangci-lint
modules/apps/transfer/keeper/relay.go
55-55: undefined: Keeper (typecheck)
174-174: undefined: Keeper (typecheck)
314-314: undefined: Keeper (typecheck)
329-329: undefined: Keeper (typecheck)
337-337: undefined: Keeper (typecheck)
381-381: undefined: Keeper (typecheck)
397-397: undefined: Keeper (typecheck)
415-415: undefined: Keeper (typecheck)
Additional comments not posted (6)
modules/apps/transfer/keeper/export_test.go (1)
35-38: Add a comment explaining the purpose of this wrapper function.+ // CreatePacketDataBytesFromVersion wraps the createPacketDataBytesFromVersion function to expose it for testing. func CreatePacketDataBytesFromVersion(appVersion, sender, receiver, memo string, tokens types.Tokens) []byte { return createPacketDataBytesFromVersion(appVersion, sender, receiver, memo, tokens) }Likely invalid or redundant comment.
modules/apps/transfer/keeper/relay.go (1)
140-140: Centralization of packet data creation logic insendTransferenhances maintainability and clarity.modules/apps/transfer/keeper/relay_test.go (4)
21-21: The importibcmock "github.com/cosmos/ibc-go/v8/testing/mock"is correctly added and used in the test cases.
1249-1306: The setup for the test functionTestCreatePacketDataBytesFromVersionis well-structured and clear, providing a good foundation for the test cases.
1255-1297: The test cases withinTestCreatePacketDataBytesFromVersionare comprehensive, covering various scenarios including success and failure cases based on the app version.
1300-1318: The execution of test cases inTestCreatePacketDataBytesFromVersionis correctly implemented, effectively handling both expected results and error scenarios.
|
|
||
| token := tokens[0] | ||
| packetData := types.NewFungibleTokenPacketData(token.Denom.Path(), token.Amount, sender, receiver, memo) | ||
| packetDataBytes = packetData.GetBytes() |
There was a problem hiding this comment.
could maybe just return early?
There was a problem hiding this comment.
no strong opinion either way, can def do if others would prefer.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- modules/apps/transfer/keeper/relay.go (2 hunks)
Additional context used
Path-based instructions (1)
modules/apps/transfer/keeper/relay.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
golangci-lint
modules/apps/transfer/keeper/relay.go
55-55: undefined: Keeper (typecheck)
174-174: undefined: Keeper (typecheck)
314-314: undefined: Keeper (typecheck)
329-329: undefined: Keeper (typecheck)
337-337: undefined: Keeper (typecheck)
381-381: undefined: Keeper (typecheck)
397-397: undefined: Keeper (typecheck)
415-415: undefined: Keeper (typecheck)
Additional comments not posted (1)
modules/apps/transfer/keeper/relay.go (1)
445-466: Ensure the new functioncreatePacketDataBytesFromVersionis adequately tested.Verification successful
The test function
TestCreatePacketDataBytesFromVersionis present and covers various scenarios, including success cases for both versions, as well as failure cases for invalid token length and invalid version. This indicates that the functioncreatePacketDataBytesFromVersionis adequately tested.
modules/apps/transfer/keeper/relay_test.go:
- Line 1249:
func (suite *KeeperTestSuite) TestCreatePacketDataBytesFromVersion() {- Line 1262-1271: Success case for version V1.
- Line 1272-1281: Success case for version V2.
- Line 1282-1290: Failure case for invalid token length in version V1.
- Line 1291-1297: Failure case for invalid version.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and adequacy of the test function for `createPacketDataBytesFromVersion`. # Test: Search for the test function. Expect: At least one occurrence of the test function. rg --type go $'TestCreatePacketDataBytesFromVersion'Length of output: 165
Script:
#!/bin/bash # Description: Extract the content of the test function `TestCreatePacketDataBytesFromVersion` to verify its adequacy. # Extract the content of the test function. ast-grep --lang go --pattern $'func (suite *KeeperTestSuite) TestCreatePacketDataBytesFromVersion() { $$$ }' modules/apps/transfer/keeper/relay_test.goLength of output: 5100
| // createPacketDataBytesFromVersion creates the packet data bytes to be sent based on the application version. | ||
| func createPacketDataBytesFromVersion(appVersion, sender, receiver, memo string, tokens types.Tokens) []byte { | ||
| var packetDataBytes []byte | ||
| switch appVersion { | ||
| case types.V1: | ||
| // Sanity check, tokens must always be of length 1 if using app version V1. | ||
| if len(tokens) != 1 { | ||
| panic(fmt.Errorf("length of tokens must be equal to 1 if using %s version", types.V1)) | ||
| } | ||
|
|
||
| token := tokens[0] | ||
| packetData := types.NewFungibleTokenPacketData(token.Denom.Path(), token.Amount, sender, receiver, memo) | ||
| packetDataBytes = packetData.GetBytes() | ||
| case types.V2: | ||
| packetData := types.NewFungibleTokenPacketDataV2(tokens, sender, receiver, memo) | ||
| packetDataBytes = packetData.GetBytes() | ||
| default: | ||
| panic(fmt.Errorf("app version must be one of %s", types.SupportedVersions)) | ||
| } | ||
|
|
||
| return packetDataBytes | ||
| } |
There was a problem hiding this comment.
Replace panic with error handling to improve robustness.
- panic(fmt.Errorf("length of tokens must be equal to 1 if using %s version", types.V1))
+ return nil, fmt.Errorf("length of tokens must be equal to 1 if using %s version", types.V1)
- panic(fmt.Errorf("app version must be one of %s", types.SupportedVersions))
+ return nil, fmt.Errorf("app version must be one of %s", types.SupportedVersions)Committable suggestion was skipped due to low confidence.
|
Description
closes: #6468
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.Summary by CodeRabbit
Refactor
sendTransferfunction by consolidating packet data creation into a new function.Tests