Support non critical opt extions in clob and sending cli #2667
Conversation
WalkthroughThis pull request introduces a new mechanism for managing transaction flags related to authenticators across multiple CLI commands in the protocol. A new file Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 0
🧹 Nitpick comments (10)
protocol/x/clob/client/cli/tx_cancel_order.go (2)
70-81: Validate authenticator ID usage.
Fetching permissioned key authenticators is a critical step. Consider validating that these authenticators exist on-chain or in your configuration to avoid runtime confusion.
87-87: Good addition of permissioned key flags.
This line ensures users can specify their authenticators via CLI flags.protocol/x/sending/client/cli/tx_create_transfer.go (1)
94-94: CLI flags for permissioned keys.
Adding CLI flags for permissioned keys is a positive step toward standardizing how authenticators are configured.protocol/x/clob/client/cli/tx_batch_cancel.go (2)
82-93: Optional TxExtension flow is well-handled.
Capturing authenticator IDs and wrapping them into TxExtension is a good approach. Consider logging a warning if no authenticators are found but the user expected them.
99-99: CLI flags for permissioned keys.
Again, adding these flags is valuable.protocol/x/clob/client/cli/tx_place_order.go (1)
106-106: Adds custom permissioned key flags.
This aligns with the new approach to handle permissioned key authenticators.protocol/x/sending/client/cli/tx_deposit_to_subaccount.go (1)
7-9: Consider renaming imports for consistency.
The new imports (codectypes, customflags, aptypes) are clear, but there's an inconsistency in the spelling of "Permisioned" (missing one 's'). Consider renaming methods and packages to "Permissioned" for better clarity.protocol/x/sending/client/cli/tx_withdraw_from_subaccount.go (1)
7-9: Keep naming consistent.
Similar to the other file, consider adjusting “Permisioned” to “Permissioned” to keep naming consistent.protocol/app/flags/permissioned_keys_flags.go (2)
7-9: Consider updating flag naming to match common usage.
"authenticators" might benefit from a more descriptive name, such as "permissioned-authenticators" or "key-authenticators," if you expect more flags in the future.
17-29: Spelling fix and extended validation recommended.
“GetPermisionedKeyAuthenticatorsForExtOptions” has a minor spelling issue in “Permisioned.” Also, consider validating IDs beyond integer conversion (e.g., ensuring numeric ranges).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
protocol/go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
protocol/app/flags/permissioned_keys_flags.go(1 hunks)protocol/go.mod(1 hunks)protocol/x/clob/client/cli/tx_batch_cancel.go(2 hunks)protocol/x/clob/client/cli/tx_cancel_order.go(2 hunks)protocol/x/clob/client/cli/tx_place_order.go(2 hunks)protocol/x/sending/client/cli/tx_create_transfer.go(2 hunks)protocol/x/sending/client/cli/tx_deposit_to_subaccount.go(2 hunks)protocol/x/sending/client/cli/tx_withdraw_from_subaccount.go(2 hunks)
🔇 Additional comments (18)
protocol/x/clob/client/cli/tx_cancel_order.go (2)
7-9: Imports look good.
These import statements appropriately bring in the new dependencies for codec types and custom flags.
65-69: Sufficient error handling in factory creation.
The creation of the transaction factory is well-handled. Ensure you propagate the error to the caller if it fails for any reason.
protocol/x/sending/client/cli/tx_create_transfer.go (3)
9-11: Imports look correct and consistent.
All new imports contribute to the updated transaction approach. No issues found.
72-76: Appropriate transaction factory creation.
Initialization of the transaction factory is consistent with the new approach, providing flexibility for extension options.
77-88: Handling non-critical extension options.
This block correctly captures and attaches permissioned key authenticator data as non-critical extension options.
protocol/x/clob/client/cli/tx_batch_cancel.go (2)
10-12: Imports introduced for extension approach.
The inclusion of these packages appears consistent with the broader move to non-critical extension options.
77-81: Consistent usage of tx.NewFactoryCLI.
This replicates the established pattern: robust error handling and factory initialization.
protocol/x/clob/client/cli/tx_place_order.go (3)
7-9: Imports introduced to support new extension structure.
No issues noted with these import lines.
84-88: Transaction factory initialization.
Consistent pattern for factory setup. The error is correctly returned if initialization fails.
89-100: TxExtension logic.
Successfully builds upon the prior pattern, adding extension options if the user supplies authenticators.
protocol/x/sending/client/cli/tx_deposit_to_subaccount.go (3)
66-71: Transaction factory creation looks correct.
The usage of tx.NewFactoryCLI(clientCtx, cmd.Flags()) properly sets up the transaction context. This is consistent with other CLI patterns. No issues found.
72-83: Validate potential edge cases for empty or invalid authenticators.
You are gracefully checking errors from GetPermisionedKeyAuthenticatorsForExtOptions. Ensure that you also have unit tests covering the scenario of empty slices and invalid authenticator IDs.
89-89: Good practice adding permissioned key flags.
Adding TxPermissionedKeyFlags ensures users can specify permissioned key authenticators. The function call is correct and aligns with the rest of the CLI flow.
protocol/x/sending/client/cli/tx_withdraw_from_subaccount.go (3)
66-71: Factory usage is aligned with CLI best practices.
The setup for transaction broadcasting through a factory approach is good. No logical errors noted.
72-83: Verify handling of invalid or partial flags.
When retrieving authenticator IDs, ensure that partial or invalid flags are tested or gracefully handled, especially if multiple flags are used.
89-89: Nicely added permissioned key flags.
It consistently follows the new pattern for CLI commands supporting permissioned key functionality.
protocol/app/flags/permissioned_keys_flags.go (1)
11-15: Flag addition logic is straightforward.
This is a standard approach for adding flags via Cobra. Function documentation is concise and clear.
protocol/go.mod (1)
475-475: Monitor security patches for the Cosmos SDK fork.
Replacing the Cosmos SDK with a fork is acceptable, but ensure you regularly merge upstream patches and verify no new security advisories are missed.
Changelist
[Describe or list the changes made in this PR]
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit