feat: implement allowed denoms #13
Conversation
WalkthroughThe recent update enhances control over denominations in a blockchain environment, focusing on forwarding functionalities. Changes include adding an Changes
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Out of diff range and nitpick comments (2)
local.sh (1)
Line range hint
1-1: Add a shebang to specify the shell interpreter explicitly.+ #!/bin/bashapi/noble/forwarding/v1/genesis.pulsar.go (1)
16-16: Ensure that the_GenesisState_1_listtype is properly documented for clarity.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (6)
api/noble/forwarding/v1/query_grpc.pb.gois excluded by!**/*.pb.goapi/noble/forwarding/v1/tx_grpc.pb.gois excluded by!**/*.pb.gox/forwarding/types/genesis.pb.gois excluded by!**/*.pb.gox/forwarding/types/query.pb.gois excluded by!**/*.pb.gox/forwarding/types/query.pb.gw.gois excluded by!**/*.pb.gw.gox/forwarding/types/tx.pb.gois excluded by!**/*.pb.go
Files selected for processing (22)
- .changelog/unreleased/features/13-allowed-denoms.md (1 hunks)
- api/noble/forwarding/module/v1/module.pulsar.go (14 hunks)
- api/noble/forwarding/v1/genesis.pulsar.go (35 hunks)
- api/noble/forwarding/v1/query.pulsar.go (33 hunks)
- api/noble/forwarding/v1/tx.pulsar.go (5 hunks)
- e2e/forwarding_test.go (9 hunks)
- local.sh (1 hunks)
- proto/noble/forwarding/module/v1/module.proto (1 hunks)
- proto/noble/forwarding/v1/genesis.proto (1 hunks)
- proto/noble/forwarding/v1/query.proto (2 hunks)
- proto/noble/forwarding/v1/tx.proto (2 hunks)
- simapp/app.yaml (1 hunks)
- x/forwarding/genesis.go (2 hunks)
- x/forwarding/keeper/keeper.go (5 hunks)
- x/forwarding/keeper/msg_server.go (2 hunks)
- x/forwarding/keeper/query_server.go (1 hunks)
- x/forwarding/keeper/state.go (1 hunks)
- x/forwarding/module.go (3 hunks)
- x/forwarding/types/codec.go (1 hunks)
- x/forwarding/types/errors.go (1 hunks)
- x/forwarding/types/genesis.go (2 hunks)
- x/forwarding/types/keys.go (1 hunks)
Files not summarized due to errors (3)
- api/noble/forwarding/v1/genesis.pulsar.go: Error: Message exceeds token limit
- api/noble/forwarding/v1/query.pulsar.go: Error: Message exceeds token limit
- api/noble/forwarding/v1/tx.pulsar.go: Error: Message exceeds token limit
Files skipped from review due to trivial changes (3)
- .changelog/unreleased/features/13-allowed-denoms.md
- simapp/app.yaml
- x/forwarding/types/keys.go
Additional Context Used
ShellCheck (1)
local.sh (1)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
Additional comments not posted (31)
x/forwarding/types/errors.go (1)
6-7: The error definitions are clear and appropriately registered.proto/noble/forwarding/module/v1/module.proto (1)
11-12: The addition of theauthorityfield is well-documented and correctly positioned.x/forwarding/genesis.go (2)
12-14: Ensure that the initialization of allowed denominations is covered by unit tests.
31-31: The export of genesis state correctly includes the newAllowedDenomsfield.x/forwarding/types/codec.go (1)
24-24: The registration ofMsgSetAllowedDenomsis correctly implemented for both legacy and new codec systems.x/forwarding/types/genesis.go (1)
13-15: The default genesis state is correctly initialized with a sensible default forAllowedDenoms.local.sh (1)
18-19: The commands for setting up theauthorityaccount are correctly implemented.proto/noble/forwarding/v1/tx.proto (2)
18-18: The addition of theSetAllowedDenomsRPC method is correctly implemented.
54-65: TheMsgSetAllowedDenomsmessage is well-defined with clear structure and appropriate protobuf options.x/forwarding/keeper/query_server.go (1)
15-23: The implementation of theDenomsfunction is correct and adheres to best practices.x/forwarding/keeper/state.go (1)
12-21: The implementation of theGetAllowedDenomsfunction is correct and adheres to best practices.proto/noble/forwarding/v1/query.proto (2)
15-18: The RPC methodDenomsis correctly defined with appropriate annotations for HTTP GET requests.
40-42: The messageQueryDenomsResponseis correctly defined with a repeated field for allowed denominations.x/forwarding/keeper/msg_server.go (1)
121-140: The implementation of theSetAllowedDenomsfunction is correct, with robust checks for authority and validation of denominations.x/forwarding/keeper/keeper.go (1)
95-104: TheIsAllowedDenommethod is correctly implemented with checks for both wildcard and specific denominations.x/forwarding/module.go (2)
157-165: The CLI commandset-allowed-denomsis correctly defined with appropriate positional arguments for setting allowed denominations.
237-248: TheProvideModulefunction correctly checks for the authority configuration and initializes theKeeperwith appropriate dependencies.e2e/forwarding_test.go (1)
305-364: TheTestAllowedDenomsfunction is correctly implemented, thoroughly testing the setting and querying of allowed denominations.api/noble/forwarding/module/v1/module.pulsar.go (3)
17-24: The addition and initialization offd_Module_authorityare correctly implemented.
92-97: The modifications to theRangemethod to handle theauthorityfield are correctly implemented.
Line range hint
113-132: The modifications to theHas,Clear,Get,Set,Mutable,NewField, andProtoMethodsmethods to handle theauthorityfield are correctly implemented.Also applies to: 149-173, 194-210, 280-319, 369-400
api/noble/forwarding/v1/genesis.pulsar.go (10)
Line range hint
310-319: The field descriptors for theGenesisStateare correctly initialized. Ensure that these fields are used consistently throughout the codebase.Verification successful
Field Descriptors Usage Limited to Declaration File
The verification process focused on the consistent usage of the field descriptors
fd_GenesisState_allowed_denoms,fd_GenesisState_num_of_accounts,fd_GenesisState_num_of_forwards, andfd_GenesisState_total_forwardedacross the codebase. The results indicate that these descriptors are used consistently within their declaration file (api/noble/forwarding/v1/genesis.pulsar.go) but do not appear to be referenced in any other parts of the codebase.This suggests that the scope of these field descriptors is limited to the specific functionalities implemented in
api/noble/forwarding/v1/genesis.pulsar.go. If this encapsulation is intentional, no further action is needed. However, if these fields are expected to be used more broadly across the codebase, this might require additional integration efforts.
- File:
api/noble/forwarding/v1/genesis.pulsar.go
- All field descriptors are declared, initialized, and used within this file.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent usage of new field descriptors across the codebase. # Test: Search for the usage of new field descriptors. Expect: Consistent usage across files. rg --type go $'fd_GenesisState_allowed_denoms|fd_GenesisState_num_of_accounts|fd_GenesisState_num_of_forwards|fd_GenesisState_total_forwarded'Length of output: 1436
Script:
#!/bin/bash # Description: Expand search to include indirect or differently formatted references to field descriptors. # Test: Search for any form of reference to the field descriptors, including possible variations in formatting. rg --type go --glob '!api/noble/forwarding/v1/genesis.pulsar.go' 'GenesisState.*allowed_denoms|GenesisState.*num_of_accounts|GenesisState.*num_of_forwards|GenesisState.*total_forwarded'Length of output: 187
390-409: TheRangemethod implementation forfastReflection_GenesisStatehandles the new fields correctly. Good use of early return to simplify logic.
429-429: TheHasmethod correctly checks for the presence of theallowed_denomsfield. This is crucial for conditional logic based on field presence.
453-454: TheClearmethod forallowed_denomscorrectly sets the field tonil. This is a standard approach for clearing slice fields in protobuf-generated code.
477-482: TheGetmethod forallowed_denomsis implemented correctly, ensuring that it returns an empty list if not set, which aligns with protobuf's handling of repeated fields.
521-524: TheSetmethod forallowed_denomscorrectly updates the field from the provided list value. This is key for ensuring data integrity when modifying the genesis state.
557-562: TheMutablemethod forallowed_denomscorrectly initializes the field if it isniland returns a mutable reference. This is essential for safely modifying repeated fields in protobuf messages.
594-596: TheNewFieldmethod forallowed_denomscorrectly creates a new, empty list, which is important for initializing fields with default values.
Line range hint
675-906: TheProtoMethodsimplementation handles the sizing and marshaling of theallowed_denomsfield efficiently, using protobuf's sizing and encoding utilities. Ensure that the marshaling respects the order of elements ifDeterministicoption is set.Verification successful
The code snippet provided in the review clearly demonstrates that the
Deterministicoption is respected in the marshaling process for various map fields (TotalForwarded,NumOfForwards,NumOfAccounts). When this option is set, the keys of these maps are sorted before marshaling, ensuring deterministic order. This aligns with the requirements mentioned in the review comment.Given this, the verification can be concluded successfully:
- The
Deterministicoption is indeed respected in the marshaling process as required.- The code snippet provided shows the implementation of this behavior explicitly.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the marshaling order respects the `Deterministic` option. # Test: Check the marshaling implementation for determinism. Expect: Deterministic order when the option is set. rg --type go $'Deterministic.*MaRsHaLmAp'Length of output: 42
957-988: The unmarshaling logic forallowed_denomscorrectly handles the wire format and appends each string to the slice, ensuring robust error handling and boundary checks.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- x/forwarding/types/genesis.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/forwarding/types/genesis.go
Per the request of the Initia team, integrators of the
x/forwardingmodule now have fine-tuned control over which denoms are automatically forwarded. Note that clearing a forwarding account to a fallback address will transfer all of the balance, regardless of allowed denoms.Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Refactor
Chores