register deprecated MsgSetVaultQuotingParams #2392
Conversation
WalkthroughThe changes in this pull request focus on the reintroduction and addition of message types related to vault operations within the DYDX protocol. Specifically, the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 1
🧹 Outside diff range and nitpick comments (13)
protocol/lib/ante/unsupported_msgs.go (3)
26-26: Approved: Addition of MsgSetVaultQuotingParams as unsupported messageThe addition of
*vaulttypes.MsgSetVaultQuotingParamsto the list of unsupported messages aligns with the PR objective of registering a deprecated message type. This change effectively makes theMsgSetVaultQuotingParamsmessage unsupported in the application.Consider adding a comment explaining why this message type is now unsupported, especially since it's marked with
nolint:staticcheck. This would help future maintainers understand the reasoning behind this change.
26-27: Consider addressing static check warningsBoth new message types are preceded by
nolint:staticcheckdirectives. While this suppresses warnings, it might hide important issues that should be addressed.Instead of using
nolintdirectives, consider addressing the underlying issues causing the static check warnings. This could involve updating deprecated types or refactoring the code to comply with the latest best practices. If there's a specific reason why these warnings can't be addressed, please add a comment explaining the rationale.
26-27: Summary of changes and potential impactThe changes to
IsUnsupportedMsgfunction add two new message types (MsgSetVaultQuotingParamsandMsgUpdateParams) to the list of unsupported messages. This aligns with the PR objective of registering deprecated message types, but also includes an additional type not mentioned in the objectives.These changes may have significant implications:
- Any part of the application using these message types will now be considered unsupported, potentially breaking existing functionality.
- The use of
nolint:staticcheckdirectives suggests there might be underlying issues that should be addressed.To ensure a smooth transition and maintain code quality:
- Document the reasons for deprecating these message types.
- Update any existing code that might be using these message types.
- Consider creating a migration plan for any affected functionality.
- Address the underlying issues causing static check warnings, if possible.
These steps will help maintain the long-term health of the codebase and ease future maintenance efforts.
protocol/x/vault/types/codec.go (1)
22-27: Approved implementation with suggestions for improvementThe implementation correctly registers the deprecated
MsgSetVaultQuotingParamsmessage type, consistent with the existing pattern in the file. However, I have a few suggestions and questions:
- Consider adding a formal deprecation notice using Go's built-in
deprecatedtag. This will provide better visibility to developers using this type. For example:// Deprecated: MsgSetVaultQuotingParams will be removed in future versions. Use [replacement] instead. registry.RegisterInterface( "/"+proto.MessageName(&MsgSetVaultQuotingParams{}), (*sdk.Msg)(nil), &MsgSetVaultQuotingParams{}, )
Could you provide more information about the deprecation timeline and any potential replacement for this message type?
Please update the PR description with more details about why this deprecated message is being registered now and any impact it might have on the system.
protocol/app/msgs/unsupported_msgs.go (2)
34-36: LGTM! Consider adding a TODO for future cleanup.The addition of
MsgSetVaultQuotingParamsto theUnsupportedMsgSamplesmap is appropriate given its deprecated status. The comment clearly indicates when it was deprecated and what replaces it.Consider adding a TODO comment to remind developers to remove this entry in a future major version update:
// MsgSetVaultQuotingParams is deprecated since v6.x and replaced by MsgSetVaultParams. // nolint:staticcheck +// TODO: Remove this entry in the next major version update after v6.x "/dydxprotocol.vault.MsgSetVaultQuotingParams": &vaulttypes.MsgSetVaultQuotingParams{},
37-39: LGTM! Consider adding line numbers and a TODO for future cleanup.The addition of
MsgUpdateParamsto theUnsupportedMsgSamplesmap is appropriate and consistent with the previous entry. The comment clearly indicates its deprecated status and replacement.For consistency with the previous entry and to aid future maintenance, consider the following changes:
- Add line numbers to the comment.
- Add a TODO comment for future cleanup.
Apply this diff:
- // MsgUpdateParams is deprecated since v6.x and replaced by MsgUpdateDefaultQuotingParams. - // nolint:staticcheck - "/dydxprotocol.vault.MsgUpdateParams": &vaulttypes.MsgUpdateParams{}, + // MsgUpdateParams is deprecated since v6.x and replaced by MsgUpdateDefaultQuotingParams. + // nolint:staticcheck + // TODO: Remove this entry in the next major version update after v6.x + "/dydxprotocol.vault.MsgUpdateParams": &vaulttypes.MsgUpdateParams{},protocol/lib/ante/nested_msg_test.go (1)
75-76: LGTM. Consider adding a comment for clarity.The addition of
MsgSetVaultQuotingParamsis consistent with the PR objective of registering a deprecated message type. The use ofnolint:staticcheckis appropriate for handling deprecated code in tests.Consider adding a brief comment explaining why this deprecated message type is being registered, similar to:
// Register deprecated message types for backwards compatibility in testsThis would provide context for future developers encountering this code.
proto/dydxprotocol/vault/tx.proto (3)
206-216: LGTM. Consider adding migration documentation.The
MsgUpdateParamsmessage type is correctly implemented and marked as deprecated. The deprecation notice and replacement information are clear.Consider adding a comment or documentation explaining how to migrate from
MsgUpdateParamstoMsgUpdateDefaultQuotingParamsto assist developers in updating their code.
218-231: LGTM. Consider adding migration documentation.The
MsgSetVaultQuotingParamsmessage type is correctly implemented and marked as deprecated. The deprecation notice and replacement information are clear.Consider adding a comment or documentation explaining how to migrate from
MsgSetVaultQuotingParamstoMsgSetVaultParamsto assist developers in updating their code.
205-231: Consider updating documentation and deprecation strategy.The addition of these deprecated message types maintains backward compatibility without affecting existing functionality. However, to ensure smooth transitions and prevent potential confusion:
- Update any relevant documentation to reflect these changes and guide users on using the new message types.
- Consider implementing a deprecation strategy, such as logging warnings when these deprecated types are used, to encourage migration to the new types.
- Plan for the eventual removal of these deprecated types in a future major version update.
protocol/app/msgs/all_msgs.go (1)
261-261: LGTM. Consider adding more context to the deprecation comment.The addition of the deprecated message type is in line with the PR objective. However, it would be helpful to provide more context about the deprecation.
Consider expanding the comment to include:
- The reason for deprecation
- When it was deprecated (e.g., which version)
- When it will be removed (if known)
- What to use instead (if applicable)
For example:
- "/dydxprotocol.vault.MsgSetVaultQuotingParams": {}, // deprecated + "/dydxprotocol.vault.MsgSetVaultQuotingParams": {}, // deprecated: replaced by MsgUpdateDefaultQuotingParams in v4.0.0, will be removed in v5.0.0indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.ts (2)
265-268: Nitpick: Enhance Deprecation DocumentationConsider using the
@deprecatedtag within the JSDoc comment to improve clarity and IDE support. For example:/** * MsgUpdateParams is the Msg/UpdateParams request type. - * Deprecated since v6.x as is replaced by MsgUpdateDefaultQuotingParams. + * @deprecated since v6.x - replaced by MsgUpdateDefaultQuotingParams. */This ensures the deprecation is clearly communicated and recognized by tooling.
292-295: Nitpick: Improve Deprecation AnnotationSimilarly, update the deprecation note for
MsgSetVaultQuotingParams:/** * MsgSetVaultQuotingParams is the Msg/SetVaultQuotingParams request type. - * Deprecated since v6.x as is replaced by MsgSetVaultParams. + * @deprecated since v6.x - replaced by MsgSetVaultParams. */Using the
@deprecatedtag enhances visibility in documentation and IDEs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
protocol/x/vault/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (8)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.ts (2 hunks)
- proto/dydxprotocol/vault/tx.proto (1 hunks)
- protocol/app/msgs/all_msgs.go (1 hunks)
- protocol/app/msgs/unsupported_msgs.go (1 hunks)
- protocol/app/msgs/unsupported_msgs_test.go (1 hunks)
- protocol/lib/ante/nested_msg_test.go (1 hunks)
- protocol/lib/ante/unsupported_msgs.go (1 hunks)
- protocol/x/vault/types/codec.go (1 hunks)
🔇 Additional comments (9)
protocol/lib/ante/unsupported_msgs.go (1)
27-27: Please clarify the addition of MsgUpdateParamsThe addition of
*vaulttypes.MsgUpdateParamsto the list of unsupported messages wasn't explicitly mentioned in the PR objectives. Could you please provide more context on why this message type is being marked as unsupported?Also, please confirm if this change is intentional and if it has any implications on the vault module's functionality.
To help verify the usage of this message type, you can run the following script:
This will help us understand where this message type is used and potentially affected by this change.
✅ Verification successful
Addition of MsgUpdateParams Confirmed as Unsupported
The addition of
*vaulttypes.MsgUpdateParamsto the list of unsupported messages has been verified. Its deprecation across multiple modules aligns with the PR objectives, and there are no active usages affecting the vault module's functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usages of MsgUpdateParams in the codebase # Test: Look for references to MsgUpdateParams rg --type go "MsgUpdateParams" -C 3Length of output: 116497
protocol/app/msgs/unsupported_msgs_test.go (1)
19-19: LGTM! Verify consistency across the codebase.The addition of "/dydxprotocol.vault.MsgSetVaultQuotingParams" to the list of unsupported messages aligns with the PR objective of registering a deprecated message type. The placement maintains the existing order within the slice.
To ensure consistency, please run the following script to check for any remaining usage of
MsgSetVaultQuotingParamsthat might need updating:This will help identify any other locations in the codebase where
MsgSetVaultQuotingParamsis used and might need to be updated or removed.✅ Verification successful
Verified consistency across the codebase.
All references to
MsgSetVaultQuotingParamsare consistently marked as deprecated or replaced, aligning with the PR objective of registering it as an unsupported message type.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any remaining usage of MsgSetVaultQuotingParams # Test: Look for MsgSetVaultQuotingParams usage rg --type go "MsgSetVaultQuotingParams"Length of output: 3640
protocol/lib/ante/nested_msg_test.go (1)
75-76: Confirm test coverage for the deprecated message type.The addition of
MsgSetVaultQuotingParamsto the map of Dydx messages ensures that this deprecated message type is included in theTestIsDydxMsg_Invalidtest cases. This maintains test coverage for backwards compatibility.To ensure comprehensive test coverage, please run the following script:
This script will help confirm that the deprecated message type is properly included in the test suite and that the relevant test function is being executed.
✅ Verification successful
Test coverage for
MsgSetVaultQuotingParamsis confirmed.The
MsgSetVaultQuotingParamsis included in the test cases withinnested_msg_test.goandunsupported_msgs.go, ensuring that deprecated message types are adequately covered in theTestIsDydxMsg_Invalidfunction.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify test coverage for MsgSetVaultQuotingParams # Test: Check if MsgSetVaultQuotingParams is used in test cases echo "Checking usage of MsgSetVaultQuotingParams in tests:" rg --type go "MsgSetVaultQuotingParams" protocol/lib/ante/ # Test: Verify if TestIsDydxMsg_Invalid is executed in the test suite echo "Verifying execution of TestIsDydxMsg_Invalid:" rg --type go "func TestIsDydxMsg_Invalid" protocol/lib/ante/ rg --type go "t.Run\(.*TestIsDydxMsg_Invalid" protocol/lib/ante/Length of output: 690
protocol/app/msgs/all_msgs.go (2)
261-268: Overall LGTM with minor suggestions.The changes to deprecate
MsgSetVaultQuotingParamsandMsgUpdateParamsin the vault module are appropriate. However, please address the following points:
- Add more context to both deprecation comments as suggested.
- Verify the intentionality and impact of deprecating
MsgUpdateParams.- Ensure that these deprecations are properly documented in the module's documentation and any relevant changelog.
These changes appear to be part of a larger refactoring or update to the vault module. It would be helpful to have a brief explanation in the PR description about the overall changes being made to the vault module and how these deprecations fit into that larger picture.
268-268: LGTM. Verify deprecation impact and consider adding more context.The deprecation of
MsgUpdateParamsis noted. However, this change wasn't explicitly mentioned in the PR objectives.Please confirm:
- Is this deprecation intentional?
- Is there a replacement message type or alternative approach?
- What is the impact on existing functionality?
As with the previous comment, consider expanding the deprecation comment to provide more context:
- "/dydxprotocol.vault.MsgUpdateParams": {}, // deprecated + "/dydxprotocol.vault.MsgUpdateParams": {}, // deprecated: [reason for deprecation], deprecated in [version], to be removed in [version], use [alternative] insteadTo help assess the impact of this deprecation, let's search for usages of this message type:
✅ Verification successful
LGTM. Deprecation of
MsgUpdateParamsconfirmed.The deprecation of
MsgUpdateParamsis verified and has been replaced byMsgUpdateDefaultQuotingParamssince v6.x. To improve clarity, consider updating the deprecation comment as follows:- "/dydxprotocol.vault.MsgUpdateParams": {}, // deprecated + "/dydxprotocol.vault.MsgUpdateParams": {}, // deprecated since v6.x, use MsgUpdateDefaultQuotingParams instead🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of MsgUpdateParams in the vault module rg --type go 'MsgUpdateParams' -C 3 protocol/x/vaultLength of output: 7309
indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.ts (4)
265-290: Reintroduction of Deprecated InterfaceMsgUpdateParamsThe interface
MsgUpdateParamshas been reintroduced and is correctly annotated with/** @deprecated */and documentation indicating deprecation since v6.x. Ensure that this reintroduction is intentional to support backward compatibility or legacy systems.
292-322: Reintroduction of Deprecated InterfaceMsgSetVaultQuotingParamsThe interface
MsgSetVaultQuotingParamshas been added back and is correctly marked as deprecated with appropriate comments. Confirm that this is necessary for backward compatibility and that any dependencies are properly managed.
1109-1163: Reintroduction of Functions forMsgUpdateParamsThe encoding, decoding, and utility functions for
MsgUpdateParamshave been reintroduced and appear to be correctly implemented. Ensure they align with the interface definitions and that their usage is properly managed given the deprecation status.
1164-1226: Reintroduction of Functions forMsgSetVaultQuotingParamsThe encoding, decoding, and utility functions for
MsgSetVaultQuotingParamshave been added back and seem correctly implemented. Verify that their inclusion is necessary and that deprecation is appropriately handled.
| // MsgSetVaultQuotingParams is deprecated since v6.x and replaced by MsgSetVaultParams. | ||
| // nolint:staticcheck | ||
| "/dydxprotocol.vault.MsgSetVaultQuotingParams": &vaulttypes.MsgSetVaultQuotingParams{}, | ||
| // MsgUpdateParams is deprecated since v6.x and replaced by MsgUpdateDefaultQuotingParams. | ||
| // nolint:staticcheck | ||
| "/dydxprotocol.vault.MsgUpdateParams": &vaulttypes.MsgUpdateParams{}, |
There was a problem hiding this comment.
💡 Codebase verification
Deprecated message types are still in use within the codebase.
The verification script identified multiple instances where the deprecated MsgSetVaultQuotingParams and MsgUpdateParams message types are still being utilized. These usages need to be updated to their respective replacement message types to ensure deprecated code is fully phased out.
Affected Areas:
protocol/x/vault/types/codec.goprotocol/x/vault/types/tx.pb.goprotocol/lib/ante/unsupported_msgs.goprotocol/app/msgs/all_msgs.goprotocol/app/msgs/internal_msgs.go- and several other files as detailed in the script output.
Please refactor these instances to use the updated message types as specified in the deprecation notes.
🔗 Analysis chain
Summary: Changes align with PR objectives and follow best practices.
The additions to the UnsupportedMsgSamples map correctly register the deprecated MsgSetVaultQuotingParams and MsgUpdateParams message types. This aligns with the PR objective of registering deprecated message types.
The changes follow best practices by:
- Clearly indicating the deprecation status and replacement messages.
- Using
nolint:staticcheckto suppress unnecessary warnings for deprecated code.
These changes effectively communicate to developers that these message types should not be used in new code while maintaining backwards compatibility.
To ensure these message types are no longer used in the codebase, run the following script:
If the script returns any results, it might indicate areas of the codebase that need to be updated to use the new message types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining usage of deprecated message types
echo "Checking for MsgSetVaultQuotingParams usage:"
rg --type go "MsgSetVaultQuotingParams" --glob '!protocol/app/msgs/unsupported_msgs.go'
echo "Checking for MsgUpdateParams usage:"
rg --type go "MsgUpdateParams" --glob '!protocol/app/msgs/unsupported_msgs.go'
Length of output: 40267
|
@Mergifyio backport release/protocol/v7.x |
✅ Backports have been created
|
(cherry picked from commit 59ca02f)
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
MsgSetVaultQuotingParams.MsgUpdateParamsandMsgUpdateParamsSDKType.MsgSetVaultQuotingParamsandMsgSetVaultQuotingParamsSDKTypeas deprecated.