fix(x/tx): add unordered and timeout timestamp fields in textual signing #23324
Conversation
📝 WalkthroughWalkthroughThis pull request introduces changes to the Changes
Sequence DiagramsequenceDiagram
participant Client
participant TxSigning
participant Envelope
Client->>TxSigning: Create Transaction
TxSigning->>Envelope: Set Unordered Flag
TxSigning->>Envelope: Set Timeout Timestamp
Envelope-->>TxSigning: Validate Transaction
TxSigning-->>Client: Signed Transaction
Possibly Related PRs
Suggested Reviewers
Finishing Touches
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
x/tx/signing/textual/internal/textualpb/textual.pulsar.go (4)
14-14: Missing Import CommentThe new import of
timestamppb "google.golang.org/protobuf/types/known/timestamppb"should be accompanied by a comment explaining why it's needed, to enhance code readability.
Line range hint
3104-3168: Update Comment forTextualDataandSignerDataThe comments for
TextualDataandSignerDatamention their roles and usage. Consider updating these comments to reflect any changes in their interactions with the newly added fields in theEnvelopemessage.For example:
// TextualData represents all the information needed to generate // the textual SignDoc (which is []Screen encoded to CBOR). It is meant to be // used as an internal type in Textual's implementations. + // It now includes support for unordered transactions and timeout timestamps.
3283-3284: Document New Fields in theEnvelopeMessageThe fields
UnorderedandTimeoutTimestamphave been added to theEnvelopemessage. It's important to update the documentation comments for theEnvelopestruct to explain the purpose and usage of these new fields.For example:
// Envelope is an internal data structure used to generate the tx envelope // screens. It is derived from the TextualData struct (also internal) which // contains the three following fields: // - body_bytes (from the original tx), // - auth_info_bytes (from the original tx), // - signer_data (passed in by the sign mode handler) // // If any of the three structs above is modified, then this Envelope message // also needs to be updated. +// //+// unordered indicates whether the transaction should be processed in an unordered fashion. //+// timeout_timestamp specifies the timestamp after which the transaction should be considered invalid.Also applies to: 3419-3431
3444-3453: Maintain Import OrderIn the imports section, ensure that standard library imports, external imports, and project-specific imports are properly grouped and ordered for better readability.
Consider grouping
google.protobufimports together.x/tx/signing/textual/tx.go (1)
Line range hint
3169-3170: UpdategetSignersFunction DocumentationThe
getSignersfunction relies on the protobuf annotationcosmos.msg.v1.signer. Ensure that this function's documentation is up to date, especially if the new fields affect signer determination.Consider adding:
// getSigners gets the ordered signers of a transaction, taking into account // the new `Unordered` field, which may impact the signing order.x/tx/signing/textual/internal/textualpb/textual.proto (2)
7-7: Group New Import with Similar DependenciesThe new import
google/protobuf/timestamp.protoshould be grouped with othergoogle.protobufimports for better organization.Consider reordering imports:
import "google/protobuf/any.proto"; import "google/protobuf/timestamp.proto";
87-88: Add Comments for New Fields inEnvelopeThe new field
bool unordered = 19;lacks a descriptive comment. Adding a comment will enhance the understanding of its purpose in the message.For example:
// unordered indicates if the transaction should be processed without order constraints. bool unordered = 19;
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
x/tx/signing/textual/internal/textualpb/buf.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
x/tx/CHANGELOG.md(1 hunks)x/tx/signing/textual/internal/textualpb/buf.yaml(1 hunks)x/tx/signing/textual/internal/textualpb/textual.proto(2 hunks)x/tx/signing/textual/internal/textualpb/textual.pulsar.go(20 hunks)x/tx/signing/textual/tx.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
x/tx/signing/textual/tx.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/tx/CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
x/tx/signing/textual/internal/textualpb/textual.pulsar.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: tests (03)
- GitHub Check: tests (02)
- GitHub Check: tests (01)
- GitHub Check: tests (00)
- GitHub Check: test-simapp-v2
- GitHub Check: test-integration
- GitHub Check: test-system-v2
- GitHub Check: build (arm64)
- GitHub Check: build (amd64)
- GitHub Check: Analyze
- GitHub Check: golangci-lint
- GitHub Check: Summary
🔇 Additional comments (14)
x/tx/signing/textual/internal/textualpb/buf.yaml (2)
3-3: Verify potential circular dependencyAdding
buf.build/cosmos/cosmos-sdkas a dependency within the cosmos-sdk repository itself might create a circular dependency. Please verify if this is intentional and necessary.✅ Verification successful
Dependency pattern is correct and consistent with codebase
The
buf.build/cosmos/cosmos-sdkdependency is a standard pattern used across the repository's protobuf configurations, appearing in 28+ other buf.yaml files. This is normal for protobuf type definitions and doesn't create problematic circular dependencies.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if this creates a circular dependency by examining other buf.yaml files echo "Searching for other buf.yaml files that might be affected..." fd buf.yaml --type f --exec grep -l "buf.build/cosmos/cosmos-sdk" {}Length of output: 1108
6-6: Verify wellknowntypes version compatibilityThe addition of
buf.build/protocolbuffers/wellknowntypes:v23.4seems appropriate for the new timestamp field. However, let's verify if this is the latest stable version and if it's compatible with other dependencies.✅ Verification successful
✓ Wellknowntypes version is consistent
The version v23.4 is consistently used across both buf.yaml files in the codebase, confirming proper version alignment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check other buf.yaml files for wellknowntypes versions to ensure consistency echo "Checking for other wellknowntypes versions in the codebase..." fd buf.yaml --type f --exec grep -l "buf.build/protocolbuffers/wellknowntypes" {}Length of output: 284
Script:
#!/bin/bash echo "=== Content of ./x/tx/signing/textual/internal/textualpb/buf.yaml ===" grep -A 1 -B 1 "wellknowntypes" ./x/tx/signing/textual/internal/textualpb/buf.yaml echo -e "\n=== Content of ./proto/buf.yaml ===" grep -A 1 -B 1 "wellknowntypes" ./proto/buf.yamlLength of output: 553
x/tx/signing/textual/internal/textualpb/textual.pulsar.go (7)
1519-1520: Ensure Consistency in Field DescriptorsThe addition of
fd_Envelope_unorderedandfd_Envelope_timeout_timestampshould be consistently integrated with proper initialization in theinit()function. Verify that all field descriptors are correctly set up to avoid runtime issues.Also applies to: 1542-1543
1707-1718: Handle New Fields in Range MethodThe
Rangemethod forfastReflection_Envelopenow includes handling forUnorderedandTimeoutTimestamp. Ensure that these fields are correctly reflected and that their inclusion does not affect the iteration logic over populated fields.
1818-1821: Clear Method Should Reset New FieldsThe
Clearmethod now resetsUnorderedandTimeoutTimestamp. Confirm that clearing these fields aligns with expected behavior and does not introduce unintended side effects.
1901-1906: Get Method Correctly Retrieves New FieldsThe
Getmethod now includes cases forUnorderedandTimeoutTimestamp. Ensure that the retrieval logic accurately returns the values of these new fields.
Line range hint
2028-2054: Mutable and NewField Methods Should Correctly Handle New FieldsIn the
MutableandNewFieldmethods, handling ofTimeoutTimestampis added. Ensure that attempting to mutate or create these fields behaves as intended and that immutability constraints are respected where applicable.
2250-2256: Update Size and Marshal Methods for New FieldsThe
SizeandMarshalmethods are updated to includeUnorderedandTimeoutTimestamp. Verify that the size calculations and marshaling logic correctly account for these fields to prevent serialization errors.Also applies to: 2286-2313
2999-3054: Update Unmarshal Method to Handle New FieldsThe
Unmarshalmethod now includes cases forUnorderedandTimeoutTimestamp. Confirm that it can correctly parse incoming data for these fields without causing errors, ensuring robustness in deserialization.x/tx/signing/textual/tx.go (1)
234-235: Ensure New Fields Are Properly Marshaled BackWhen reconstructing
txBodyin theParsemethod,TimeoutTimestampandUnorderedare included. Verify that these fields are correctly populated from the parsedenvelopeto maintain data integrity during the serialization and deserialization processes.x/tx/signing/textual/internal/textualpb/textual.proto (1)
87-88: Confirm Field Numbers Do Not ConflictEnsure that the field numbers assigned to
unordered(19) andtimeout_timestamp(20) do not conflict with any existing or reserved field numbers in theEnvelopemessage.Run the following script to verify field numbers:
If the script outputs any numbers, there is a conflict that needs to be resolved.
✅ Verification successful
No Field Number Conflicts Found
The field numbers 19 and 20 assigned to
unorderedandtimeout_timestamprespectively do not conflict with any existing fields in theEnvelopemessage.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for duplicate field numbers in Envelope message. # Extract field numbers grep -E '=[[:space:]]*[0-9]+;' textual.proto | grep -o '=[[:space:]]*[0-9]\+' | sort | uniq -dLength of output: 143
Script:
#!/bin/bash # First, let's verify the file exists and see its content cat x/tx/signing/textual/internal/textualpb/textual.proto # Then use ast-grep to find the Envelope message structure ast-grep --pattern 'message Envelope { $$$ }'Length of output: 3984
x/tx/CHANGELOG.md (3)
36-38: LGTM! Version and release date are properly formatted.The new version entry follows the changelog format guidelines with proper linking and release date.
38-38: LGTM! Changelog message is clear and follows the format.The message correctly references PR #23324 and clearly describes the changes related to signing over unordered and timeout timestamp fields.
40-42: LGTM! Version note is clear and accurate.The note about v1.0.0 being identical to v1.0.0-alpha.3 provides helpful context about version equivalence.
| case "Envelope.unordered": | ||
| x.Unordered = value.Bool() | ||
| case "Envelope.timeout_timestamp": | ||
| x.TimeoutTimestamp = value.Message().Interface().(*timestamppb.Timestamp) |
There was a problem hiding this comment.
Set Method Should Handle New Fields Safely
The Set method now allows setting values for Unordered and TimeoutTimestamp. Validate that input values are correctly assigned and that type assertions are safe to prevent potential panics.
Apply this diff to ensure type safety when setting the TimeoutTimestamp field:
- x.TimeoutTimestamp = value.Message().Interface().(*timestamppb.Timestamp)
+ if ts, ok := value.Message().Interface().(*timestamppb.Timestamp); ok {
+ x.TimeoutTimestamp = ts
+ } else {
+ panic(fmt.Errorf("invalid type for TimeoutTimestamp field"))
+ }📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case "Envelope.unordered": | |
| x.Unordered = value.Bool() | |
| case "Envelope.timeout_timestamp": | |
| x.TimeoutTimestamp = value.Message().Interface().(*timestamppb.Timestamp) | |
| case "Envelope.unordered": | |
| x.Unordered = value.Bool() | |
| case "Envelope.timeout_timestamp": | |
| if ts, ok := value.Message().Interface().(*timestamppb.Timestamp); ok { | |
| x.TimeoutTimestamp = ts | |
| } else { | |
| panic(fmt.Errorf("invalid type for TimeoutTimestamp field")) | |
| } |
| TimeoutTimestamp: txBody.TimeoutTimestamp, | ||
| Unordered: txBody.Unordered, |
There was a problem hiding this comment.
Handle Potential Nil Pointers for New Fields
When assigning TimeoutTimestamp and Unordered from txBody, ensure that txBody is not nil and that these fields are properly initialized to avoid potential nil pointer dereferences.
Apply this diff to add nil checks:
+ if txBody != nil {
envelope.TimeoutTimestamp = txBody.TimeoutTimestamp
envelope.Unordered = txBody.Unordered
+ }📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| TimeoutTimestamp: txBody.TimeoutTimestamp, | |
| Unordered: txBody.Unordered, | |
| if txBody != nil { | |
| TimeoutTimestamp: txBody.TimeoutTimestamp, | |
| Unordered: txBody.Unordered, | |
| } |
Description
Found during #22723
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Improvements
Chores