Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix pulsar and some typos in proto #193

Merged
merged 2 commits into from
Jun 7, 2024
Merged

fix pulsar and some typos in proto #193

merged 2 commits into from
Jun 7, 2024

Conversation

Vritra4
Copy link
Contributor

@Vritra4 Vritra4 commented May 31, 2024

  • fix protocgen-pulsar.sh
    • remove fetchprice
  • add api/ibc/applications/nft_transfer
  • fix typos

@Vritra4 Vritra4 requested a review from beer-1 May 31, 2024 06:08
@Vritra4 Vritra4 requested a review from a team as a code owner May 31, 2024 06:08
Copy link

coderabbitai bot commented May 31, 2024

Walkthrough

Walkthrough

This update primarily focuses on correcting typographical errors in comments across various files, enhancing the clarity and readability of the code documentation. Additionally, there are minor changes in functionality, such as updating the return values in GetUpgradePolicy functions and modifying directories in a script. These changes improve the overall quality of the codebase without altering its fundamental behavior.

Changes

Files/Paths Change Summary
api/initia/distribution/v1/genesis.pulsar.go Corrected typos in comments related to validator rewards and commissions.
api/initia/distribution/v1/query.pulsar.go Corrected a typo in a comment regarding validator commission.
api/initia/move/v1/tx.pulsar.go Updated GetUpgradePolicy functions to return UpgradePolicy_UNSPECIFIED instead of ARBITRARY.
api/initia/mstaking/v1/staking.pulsar.go Fixed a typo in a comment regarding validator infractions.
proto/initia/distribution/v1/genesis.proto Corrected typos in comments related to validator rewards, slash events, and commissions.
proto/initia/distribution/v1/query.proto Fixed a typo in a comment related to validator commission.
proto/initia/gov/v1/gov.proto Corrected a comment related to Move function identifiers.
proto/initia/mstaking/v1/staking.proto Corrected a typo in a comment about validator infractions.
scripts/protocgen-pulsar.sh Updated directories searched for .proto files, including a new directory and excluding another.
x/move/client/cli/tx.go Corrected a typo in the command description related to the upload limit of binary files.
app/app.go Fixed a typo in a comment about adding the oracle keeper as a hook.
x/ibc-hooks/move-hooks/README.md Corrected a typo in the documentation related to ICS20 packet data propagation.

Poem

Amidst the code where comments play,
Typographical errors had their sway.
With careful eyes and nimble paws,
We fixed the lines, addressed the flaws.
Now clearer paths for all to see,
The code's as fine as it can be.
Hopping forward, bugs at bay! 🐇✨


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 9

Outside diff range and nitpick comments (5)
api/ibc/applications/nft_transfer/v1/types.pulsar.go (1)

979-991: Ensure proper documentation for fields in ClassTrace.

Consider adding more detailed comments for the fields path and base_class_id to explain their usage and importance more clearly.

api/initia/move/v1/types.pulsar.go (4)

17-59: Consider documenting the _Params_4_list type and its methods to improve code maintainability and clarity for other developers.


17-59: The methods in _Params_4_list are implemented correctly. However, consider using more descriptive variable names than x for instances of _Params_4_list to enhance code readability.


17-59: The UpgradePolicy enum and its mappings are correctly defined. Consider adding documentation for each enum value to clarify their usage contexts.


17-59: The struct definitions and protobuf field tags in hunk 16 are correctly formatted. Ensure that these fields are appropriately indexed in the database if they are stored, to optimize query performance.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 85707ca and 826d2c5.

Files ignored due to path filters (6)
  • api/ibc/applications/fetchprice/v1/query_grpc.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • api/ibc/applications/nft_transfer/v1/query_grpc.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • api/ibc/applications/nft_transfer/v1/tx_grpc.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • x/distribution/types/genesis.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • x/distribution/types/query.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • x/mstaking/types/staking.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
Files selected for processing (13)
  • api/ibc/applications/nft_transfer/v1/genesis.pulsar.go (19 hunks)
  • api/ibc/applications/nft_transfer/v1/types.pulsar.go (1 hunks)
  • api/initia/distribution/v1/genesis.pulsar.go (3 hunks)
  • api/initia/distribution/v1/query.pulsar.go (1 hunks)
  • api/initia/move/v1/tx.pulsar.go (2 hunks)
  • api/initia/move/v1/types.pulsar.go (21 hunks)
  • api/initia/mstaking/v1/staking.pulsar.go (1 hunks)
  • proto/initia/distribution/v1/genesis.proto (3 hunks)
  • proto/initia/distribution/v1/query.proto (1 hunks)
  • proto/initia/gov/v1/gov.proto (1 hunks)
  • proto/initia/mstaking/v1/staking.proto (1 hunks)
  • scripts/protocgen-pulsar.sh (1 hunks)
  • x/move/client/cli/tx.go (1 hunks)
Files not summarized due to errors (3)
  • api/ibc/applications/nft_transfer/v1/genesis.pulsar.go: Error: Message exceeds token limit
  • api/ibc/applications/nft_transfer/v1/types.pulsar.go: Error: Message exceeds token limit
  • api/initia/move/v1/types.pulsar.go: Error: Message exceeds token limit
Files skipped from review due to trivial changes (8)
  • api/initia/distribution/v1/genesis.pulsar.go
  • api/initia/distribution/v1/query.pulsar.go
  • api/initia/mstaking/v1/staking.pulsar.go
  • proto/initia/distribution/v1/genesis.proto
  • proto/initia/distribution/v1/query.proto
  • proto/initia/gov/v1/gov.proto
  • proto/initia/mstaking/v1/staking.proto
  • x/move/client/cli/tx.go
Additional context used
Shellcheck
scripts/protocgen-pulsar.sh

[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.


[warning] 28-28: For loops over find output are fragile. Use find -exec or a while read loop.


[info] 29-29: Double quote to prevent globbing and word splitting.

Additional comments not posted (13)
api/ibc/applications/nft_transfer/v1/genesis.pulsar.go (1)

Line range hint 2-832: The newly added file for nft_transfer module appears correctly generated and aligns with the PR objectives.

api/ibc/applications/nft_transfer/v1/types.pulsar.go (6)

993-1000: Review the Reset method implementation for ClassTrace.

The Reset method correctly resets the ClassTrace instance to its zero value and reinitializes it with the message info. This is standard practice for protobuf messages.


1027-1037: Review the Params message definition.

The Params message is well-defined with clear field names and protobuf tags. The use of json tags alongside protobuf tags is correctly implemented.


1039-1045: Check the Reset method for Params.

The Reset method for Params follows the standard protobuf pattern, ensuring the message is reset to its default state correctly.


1060-1063: Validate getter methods for Params.

The getter methods for SendEnabled and ReceiveEnabled are implemented correctly, providing safe access to the fields with appropriate nil checks.


520-531: Review reflection methods for Params.

The reflection methods are implemented correctly, allowing for runtime manipulation and inspection of Params instances in a manner consistent with protobuf best practices.


774-800: Check the custom ProtoMethods implementation for Params.

The ProtoMethods provides optimized implementations for various protobuf operations. This customization is advanced usage and is implemented correctly here.

api/initia/move/v1/types.pulsar.go (4)

17-59: Initialization of md_Params and related descriptors is done correctly. Ensure that these descriptors are used consistently across the application to avoid discrepancies in field access.

Verification successful

The field descriptors fd_Params_base_denom, fd_Params_base_min_gas_price, fd_Params_contract_shared_revenue_ratio, and fd_Params_allowed_publishers are initialized and used consistently within the types.pulsar.go file. There are no discrepancies in their usage across the application.

  • fd_Params_base_denom is initialized and used correctly.
  • fd_Params_base_min_gas_price is initialized and used correctly.
  • fd_Params_contract_shared_revenue_ratio is initialized and used correctly.
  • fd_Params_allowed_publishers is initialized and used correctly.
Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify consistent usage of field descriptors across the application.
# Test: Search for inconsistent descriptor usage.
rg --type go 'fd_Params_' ./api/initia/move/v1/

Length of output: 1288


17-59: The use of protoreflect.ValueOfList in handling AllowedPublishers is appropriate. Ensure that the list operations are tested for performance implications if AllowedPublishers can grow large.


17-59: The encoding and decoding logic in hunks 10-13 appears correct. Verify that it aligns with the protobuf specification and that edge cases are handled in tests.


17-59: The byte manipulation and encoding logic in hunks 20-21 are complex. Ensure that they are well-documented and covered by unit tests to prevent regressions.

api/initia/move/v1/tx.pulsar.go (2)

14635-14635: Change in default return value to UpgradePolicy_UNSPECIFIED looks good.


14129-14129: Change in default return value to UpgradePolicy_UNSPECIFIED looks good.

Verification successful

To ensure the change in the default return value to UpgradePolicy_UNSPECIFIED does not introduce any issues, we need to investigate the usages of GetUpgradePolicy in the files listed in the rg output.


The investigation of the usages of GetUpgradePolicy in the listed files shows that the function is used in various contexts, including within the CodeKeeper and Module structures. The change to return UpgradePolicy_UNSPECIFIED when x is nil appears consistent across these usages.

However, to ensure there are no adverse effects, it is crucial to verify that the new default return value does not cause any logical issues or unexpected behavior in the system. This involves checking the handling of UpgradePolicy_UNSPECIFIED in the codebase.


The search results indicate that UpgradePolicy_UNSPECIFIED is handled in various parts of the codebase, including conditional checks and error returns. This suggests that the system is designed to handle this value appropriately.

Given this context, it appears that the change to return UpgradePolicy_UNSPECIFIED when x is nil should not introduce any adverse effects.

  • x/move/types/tx.go: Conditional checks for UpgradePolicy_UNSPECIFIED.
  • x/move/keeper/code.go: Returns UpgradePolicy_UNSPECIFIED in error cases.
  • api/initia/move/v1/types.pulsar.go: Defines UpgradePolicy_UNSPECIFIED.

These usages indicate that the system is prepared to handle UpgradePolicy_UNSPECIFIED correctly.

Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify that the new default return value does not affect other system parts.

# Test: Search for other usages of `GetUpgradePolicy` in the codebase. Expect: No adverse effects.
rg --type go $'GetUpgradePolicy'

Length of output: 863



Script:

#!/bin/bash
# Description: Investigate the usages of `GetUpgradePolicy` in the listed files to ensure the changes are safe.

# Display the relevant lines around the usages of `GetUpgradePolicy` in each file.
rg --type go --context 5 $'GetUpgradePolicy' x/move/keeper/querier.go x/move/keeper/keeper.go x/move/keeper/code.go api/initia/move/v1/types.pulsar.go api/initia/move/v1/tx.pulsar.go

Length of output: 4092



Script:

#!/bin/bash
# Description: Verify handling of `UpgradePolicy_UNSPECIFIED` in the codebase to ensure no adverse effects.

# Search for handling of `UpgradePolicy_UNSPECIFIED` in the codebase.
rg --type go 'UpgradePolicy_UNSPECIFIED'

Length of output: 1138

@@ -23,7 +23,7 @@ echo "Generating API module"

# exclude ibc modules
cd proto
proto_dirs=$(find ./initia ./ibc/applications/fetchprice ./ibc/applications/perm -path -prune -o -name '*.proto' -print0 | xargs -0 -n1 dirname | sort | uniq)
proto_dirs=$(find ./initia ./ibc/applications/perm ./ibc/applications/nft_transfer -path -prune -o -name '*.proto' -print0 | xargs -0 -n1 dirname | sort | uniq)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The modification to exclude ibc/applications/nft_transfer aligns with the PR objectives. Consider using find -exec or a while read loop for more robust directory processing.

find ./initia ./ibc/applications/perm ./ibc/applications/nft_transfer -path -prune -o -name '*.proto' -print0 | xargs -0 -n1 dirname | sort | uniq | while read dir; do
  for file in $(find "${dir}" -maxdepth 1 -name '*.proto'); do
    buf generate --template buf.gen.pulsar.yaml $file
  done
done

@@ -23,7 +23,7 @@

# exclude ibc modules
cd proto
proto_dirs=$(find ./initia ./ibc/applications/fetchprice ./ibc/applications/perm -path -prune -o -name '*.proto' -print0 | xargs -0 -n1 dirname | sort | uniq)
proto_dirs=$(find ./initia ./ibc/applications/perm ./ibc/applications/nft_transfer -path -prune -o -name '*.proto' -print0 | xargs -0 -n1 dirname | sort | uniq)
for dir in $proto_dirs; do
for file in $(find "${dir}" -maxdepth 1 -name '*.proto'); do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a for loop over find output is fragile. Consider using find -exec for more robust handling.

find ./initia ./ibc/applications/perm ./ibc/applications/nft_transfer -path -prune -o -name '*.proto' -print0 | xargs -0 -n1 dirname | sort | uniq | while read dir; do
  find "${dir}" -maxdepth 1 -name '*.proto' -exec buf generate --template buf.gen.pulsar.yaml {} \;
done
Tools
Shellcheck

[warning] 28-28: For loops over find output are fragile. Use find -exec or a while read loop.

@@ -14,57 +14,56 @@ import (
sync "sync"
)

var _ protoreflect.List = (*_Params_5_list)(nil)
var _ protoreflect.List = (*_Params_4_list)(nil)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation of _Params_4_list seems correct and adheres to the protoreflect.List interface. However, consider adding error handling or validation in methods like Set and Append to ensure the index is within bounds and the value is valid.

Also applies to: 19-19, 23-23, 30-30, 34-34, 40-40, 46-46, 50-50, 54-54, 59-59

Comment on lines +17 to +59
var _ protoreflect.List = (*_Params_4_list)(nil)

type _Params_5_list struct {
type _Params_4_list struct {
list *[]string
}

func (x *_Params_5_list) Len() int {
func (x *_Params_4_list) Len() int {
if x.list == nil {
return 0
}
return len(*x.list)
}

func (x *_Params_5_list) Get(i int) protoreflect.Value {
func (x *_Params_4_list) Get(i int) protoreflect.Value {
return protoreflect.ValueOfString((*x.list)[i])
}

func (x *_Params_5_list) Set(i int, value protoreflect.Value) {
func (x *_Params_4_list) Set(i int, value protoreflect.Value) {
valueUnwrapped := value.String()
concreteValue := valueUnwrapped
(*x.list)[i] = concreteValue
}

func (x *_Params_5_list) Append(value protoreflect.Value) {
func (x *_Params_4_list) Append(value protoreflect.Value) {
valueUnwrapped := value.String()
concreteValue := valueUnwrapped
*x.list = append(*x.list, concreteValue)
}

func (x *_Params_5_list) AppendMutable() protoreflect.Value {
func (x *_Params_4_list) AppendMutable() protoreflect.Value {
panic(fmt.Errorf("AppendMutable can not be called on message Params at list field AllowedPublishers as it is not of Message kind"))
}

func (x *_Params_5_list) Truncate(n int) {
func (x *_Params_4_list) Truncate(n int) {
*x.list = (*x.list)[:n]
}

func (x *_Params_5_list) NewElement() protoreflect.Value {
func (x *_Params_4_list) NewElement() protoreflect.Value {
v := ""
return protoreflect.ValueOfString(v)
}

func (x *_Params_5_list) IsValid() bool {
func (x *_Params_4_list) IsValid() bool {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The panic message in AppendMutable is clear, but it might be beneficial to also log the error or handle it more gracefully depending on the application's error handling strategy.


The IsValid method is a good safety check. Consider adding similar checks in other methods to prevent operations on a nil list.


The getter methods in hunks 17-19 are simple and effective. Consider adding null checks or default values to handle potential nil references more gracefully.


The panic statements in hunk 8 for immutable fields are clear. Ensure that these are part of a broader error handling strategy that includes logging and user feedback.


Tip

Codebase Verification

The search results indicate that there are no direct unit tests covering the _Params_4_list methods or the switch cases involving Params. This lack of test coverage is a concern and should be addressed to ensure the robustness of the code.

  • Ensure that unit tests are written to cover all methods of _Params_4_list.
  • Verify that the switch cases involving Params are adequately tested, including default cases and error handling.
Analysis chain

The switch cases in hunks 4-9 handle various fields correctly. However, ensure that all cases are covered in unit tests, especially the default cases and error handling.

Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify coverage of switch cases in unit tests.
# Test: Search for tests covering switch cases.
rg --type go 'Test.*switch.*Params' ./api/initia/move/v1/

Length of output: 59



Script:

#!/bin/bash
# Description: Verify coverage of switch cases in unit tests.
# Test: Search for tests covering switch cases in the entire repository.
rg --type go 'switch.*Params' --context 10

Length of output: 39835


Script:

#!/bin/bash
# Description: Verify the presence of unit tests for the _Params_4_list methods.
# Test: Search for unit tests that cover the methods of _Params_4_list.
rg --type go 'Test.*_Params_4_list' --context 10

Length of output: 50


Tip

Codebase Verification

The search for unit tests related to _Params_4_list did not yield any results, and no references to _Params_4_list were found in the test files. This indicates that there might be a lack of direct unit tests for this type and its methods.

  • Ensure comprehensive unit tests cover edge cases such as null list, out-of-bounds access, and type mismatches.
Analysis chain

The _Params_4_list type and its methods are well implemented. Ensure comprehensive unit tests cover edge cases such as null list, out-of-bounds access, and type mismatches.

Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify unit tests for _Params_4_list methods.
# Test: Search for unit tests related to _Params_4_list.
rg --type go 'Test.*_Params_4_list' ./api/initia/move/v1/

Length of output: 59



Script:

#!/bin/bash
# Description: Search for Go test files in the directory.
fd --type f --extension go --exec grep -l '_test.go' {}

# Description: Search for references to _Params_4_list in Go test files.
rg '_Params_4_list' --type go --glob '*_test.go'

Length of output: 137

Copy link
Member

@beer-1 beer-1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems test failed

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Outside diff range and nitpick comments (2)
scripts/protocgen-pulsar.sh (1)

Line range hint 1-1: Add a shebang to specify the shell interpreter.

The script lacks a shebang line, which is crucial for ensuring that the script runs under the correct shell environment. Consider adding the following at the top of your script:

#!/bin/bash
Tools
Shellcheck

[warning] 28-28: For loops over find output are fragile. Use find -exec or a while read loop.


[info] 29-29: Double quote to prevent globbing and word splitting.

x/ibc-hooks/move-hooks/README.md (1)

Line range hint 187-187: Correct the grammatical error in the documentation.

There's a grammatical mistake in the description of the async callback. It should be corrected as follows:

- Also when a contract make IBC transfer request, it should provide async callback data through memo field.
+ Also, when a contract makes an IBC transfer request, it should provide async callback data through the memo field.
Tools
LanguageTool

[style] ~97-~97: Try using a synonym here to strengthen your writing.
Context: ..."]} ``` ### ICS20 packet structure So given the details above, we propagate the imp...

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 826d2c5 and e7f9bed.

Files ignored due to path filters (17)
  • api/ibc/applications/perm/v1/query_grpc.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • api/ibc/applications/perm/v1/tx_grpc.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • api/initia/bank/v1/tx_grpc.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • api/initia/distribution/v1/query_grpc.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • api/initia/distribution/v1/tx_grpc.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • api/initia/gov/v1/query_grpc.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • api/initia/gov/v1/tx_grpc.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • api/initia/ibchooks/v1/query_grpc.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • api/initia/ibchooks/v1/tx_grpc.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • api/initia/intertx/v1/query_grpc.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • api/initia/intertx/v1/tx_grpc.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • api/initia/move/v1/query_grpc.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • api/initia/move/v1/tx_grpc.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • api/initia/mstaking/v1/query_grpc.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • api/initia/mstaking/v1/tx_grpc.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • api/initia/reward/v1/query_grpc.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • api/initia/reward/v1/tx_grpc.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
Files selected for processing (3)
  • app/app.go (1 hunks)
  • scripts/protocgen-pulsar.sh (1 hunks)
  • x/ibc-hooks/move-hooks/README.md (1 hunks)
Files skipped from review due to trivial changes (1)
  • app/app.go
Additional context used
Shellcheck
scripts/protocgen-pulsar.sh

[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.


[warning] 28-28: For loops over find output are fragile. Use find -exec or a while read loop.


[info] 29-29: Double quote to prevent globbing and word splitting.

LanguageTool
x/ibc-hooks/move-hooks/README.md

[style] ~9-~9: The phrase “a variety of” may be wordy. To make your writing clearer, consider replacing it.
Context: ...olve token movement. This is useful for a variety of use cases. One of primary importance is...


[style] ~10-~10: As an alternative to the over-used intensifier ‘extremely’, consider replacing this phrase.
Context: ...portance is cross-chain swaps, which is an extremely powerful primitive. The mechanism enabling this...


[style] ~97-~97: Try using a synonym here to strengthen your writing.
Context: ..."]} ``` ### ICS20 packet structure So given the details above, we propagate the imp...


[style] ~139-~139: Consider removing “of” to be more concise
Context: ...acket as directed towards movehooks iff all of the following hold: - memo is not blank ...


[grammar] ~187-~187: Possibly, a comma is missing after an introductory clause, the verb inflection is not correct, or a question mark is missing. Did you mean “contract, make” or “contract makes”?
Context: ... callback_id: u64, ) ``` Also when a contract make IBC transfer request, it should provide...


[uncategorized] ~189-~189: Loose punctuation mark.
Context: ...- memo['move']['async_callback']['id']: the async callback id is assigned from...


[uncategorized] ~190-~190: Loose punctuation mark.
Context: ...e']['async_callback']['module_address']`: The address of module which defines the...


[uncategorized] ~191-~191: Loose punctuation mark.
Context: ...move']['async_callback']['module_name']`: The name of module which defines the ca...


[uncategorized] ~191-~191: Possible missing article found.
Context: ...callback']['module_name']`: The name of module which defines the callback function.

Additional comments not posted (1)
scripts/protocgen-pulsar.sh (1)

26-26: Consider using find -exec for more robust directory processing.

This is a repeat of a previous suggestion to enhance the robustness of the script by replacing the for loop with find -exec. Here's the recommended change:

find ./initia ./ibc/applications/perm -path -prune -o -name '*.proto' -print0 | xargs -0 -n1 dirname | sort | uniq | while read dir; do
  find "${dir}" -maxdepth 1 -name '*.proto' -exec buf generate --template buf.gen.pulsar.yaml {} \;
done

@@ -23,7 +23,7 @@ echo "Generating API module"

# exclude ibc modules
cd proto
proto_dirs=$(find ./initia ./ibc/applications/fetchprice ./ibc/applications/perm -path -prune -o -name '*.proto' -print0 | xargs -0 -n1 dirname | sort | uniq)
proto_dirs=$(find ./initia ./ibc/applications/perm -path -prune -o -name '*.proto' -print0 | xargs -0 -n1 dirname | sort | uniq)
for dir in $proto_dirs; do
for file in $(find "${dir}" -maxdepth 1 -name '*.proto'); do
buf generate --template buf.gen.pulsar.yaml $file
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure variables are properly quoted to prevent word splitting.

In the script, the variable $file is used without quotes, which might lead to issues if the file names contain spaces. Modify the line as follows:

-    buf generate --template buf.gen.pulsar.yaml $file
+    buf generate --template buf.gen.pulsar.yaml "$file"
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.

Suggested change
buf generate --template buf.gen.pulsar.yaml $file
buf generate --template buf.gen.pulsar.yaml "$file"
Tools
Shellcheck

[info] 29-29: Double quote to prevent globbing and word splitting.

@@ -94,7 +94,7 @@ msg := MsgExecuteContract{

### ICS20 packet structure

So given the details above, we propogate the implied ICS20 packet data structure.
So given the details above, we propagate the implied ICS20 packet data structure.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarify the description of the ICS20 packet structure.

The description of the ICS20 packet structure could be enhanced for better clarity. Consider rephrasing to explicitly state the role of each field in the context of movehooks:

- So given the details above, we propagate the implied ICS20 packet data structure.
+ Based on the above details, here is how the ICS20 packet data structure is utilized in movehooks:
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.

Suggested change
So given the details above, we propagate the implied ICS20 packet data structure.
So given the details above, we propagate the implied ICS20 packet data structure.
Based on the above details, here is how the ICS20 packet data structure is utilized in movehooks:
Tools
LanguageTool

[style] ~97-~97: Try using a synonym here to strengthen your writing.
Context: ..."]} ``` ### ICS20 packet structure So given the details above, we propagate the imp...

Copy link

codecov bot commented Jun 3, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 38.94%. Comparing base (85707ca) to head (e7f9bed).
Report is 3 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #193      +/-   ##
==========================================
+ Coverage   37.81%   38.94%   +1.13%     
==========================================
  Files         251      251              
  Lines       24115    24131      +16     
==========================================
+ Hits         9119     9399     +280     
+ Misses      13523    13219     -304     
- Partials     1473     1513      +40     
Files Coverage Δ
app/app.go 87.24% <100.00%> (ø)
x/move/client/cli/tx.go 0.00% <0.00%> (ø)

... and 9 files with indirect coverage changes

@Vritra4
Copy link
Contributor Author

Vritra4 commented Jun 3, 2024

Seems test failed

removed pulsar for nft_transfer!

Copy link
Member

@beer-1 beer-1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@beer-1 beer-1 merged commit 4a5a1a1 into main Jun 7, 2024
7 checks passed
@beer-1 beer-1 deleted the chore/fix-pulsar branch June 7, 2024 04:16
@coderabbitai coderabbitai bot mentioned this pull request Sep 24, 2024
11 tasks
@coderabbitai coderabbitai bot mentioned this pull request Nov 14, 2024
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants