refactor(client): change flag for unordered tx timeouts #24561
Conversation
Ironbird - launch a networkTo use Ironbird, you can use the following commands:
Custom Load Test ConfigurationYou can provide a custom load test configuration using the `--load-test-config=` flag:Use |
📝 WalkthroughWalkthroughThe changes replace the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant TxFactory
participant Blockchain
User->>CLI: Submit tx with --timeout-duration
CLI->>TxFactory: Parse timeout-duration flag
TxFactory->>TxFactory: Compute timeoutTimestamp = now + duration
TxFactory->>Blockchain: Broadcast transaction with computed timeoutTimestamp
Blockchain-->>CLI: Accept or reject transaction based on timeout
CLI-->>User: Return result
sequenceDiagram
participant Test
participant CLI
participant FileSystem
participant Blockchain
Test->>CLI: Generate unsigned tx (--generate-only, --timeout-duration)
CLI->>FileSystem: Write unsigned tx to temp file
Test->>CLI: Sign tx (input: unsigned tx file)
CLI->>FileSystem: Write signed tx to temp file
Test->>CLI: Broadcast signed tx file
CLI->>Blockchain: Broadcast transaction
Blockchain-->>CLI: Return broadcast result
CLI-->>Test: Output result
Test->>CLI: Broadcast same signed tx file again
CLI->>Blockchain: Broadcast duplicate transaction
Blockchain-->>CLI: Return duplicate error
CLI-->>Test: Output duplicate error
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.md(1 hunks)client/flags/flags.go(3 hunks)client/tx/factory.go(1 hunks)systemtests/cli.go(1 hunks)tests/systemtests/unordered_tx_test.go(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
client/tx/factory.go (1)
client/flags/flags.go (1)
TimeoutDuration(77-77)
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: test-system-legacy
- GitHub Check: test-system
- GitHub Check: tests (03)
- GitHub Check: tests (02)
- GitHub Check: tests (01)
- GitHub Check: tests (00)
- GitHub Check: test-sim-nondeterminism
- GitHub Check: test-e2e
- GitHub Check: test-integration
- GitHub Check: build (arm64)
- GitHub Check: build (amd64)
- GitHub Check: Analyze
- GitHub Check: Gosec
- GitHub Check: golangci-lint
- GitHub Check: Summary
🔇 Additional comments (12)
client/flags/flags.go (3)
77-77: Good improvement to the flag namingRenaming from
FlagTimeoutTimestamptoTimeoutDurationbetter reflects the actual parameter users will provide (a duration rather than an absolute timestamp).
140-142: Good usability improvement for transaction timeout flagsThe changes improve user experience by:
- Clearly marking the timestamp approach as deprecated
- Changing to a duration-based parameter which is more intuitive for users
- Updating the description to clearly explain the behavior
This change makes the API more intuitive - users can specify "how long" a transaction should be valid rather than calculating a specific Unix timestamp.
152-153: Appropriate constraint updates for the new flagCorrectly updated the mutual exclusivity and required-together constraints to use the new
TimeoutDurationflag, maintaining the same logical relationships between flags.CHANGELOG.md (1)
69-69: Well-documented change in the CHANGELOGThe entry clearly explains the functional change: timeout timestamps for unordered transactions are now calculated by adding the specified duration to the current time. This is properly categorized as an improvement since it enhances usability.
systemtests/cli.go (1)
198-198: Good consistency improvement for CLI command executionAdding keyring flags to all commands executed through
RunCommandWithArgsensures consistent behavior across the testing framework when dealing with transactions. This supports the broader changes to transaction timeout handling.client/tx/factory.go (1)
91-95: Well-implemented timeout calculation logicThe implementation elegantly handles the new timeout duration paradigm:
- Reads the duration value from command flags
- Only calculates a timeout timestamp when a positive duration is provided
- Sets the timestamp by adding the duration to the current time
This approach is more intuitive for users as they can specify "how long" a transaction should be valid rather than calculating an absolute Unix timestamp.
tests/systemtests/unordered_tx_test.go (6)
10-10: Appropriate import change for new file operationsThe import change from
asserttotestutilaligns with the new approach of using temporary files for transaction processing.
20-21: Improved test description clarityThe updated comment clarifies that rejection happens due to the same unordered nonce rather than implying it's due to the same transaction hash, making the test's purpose more explicit.
35-40: Good implementation of multi-step transaction flow with duration-based timeoutThe code now follows a better practice of separating transaction generation from signing and broadcasting, while also implementing the new
--timeout-durationflag instead of--timeout-timestamp. The temporary file handling is also properly implemented with error checking.
46-48: Clean transaction broadcast implementationThe broadcast step is correctly implemented, leveraging the previously signed transaction file.
50-56: Improved error verification for duplicate transactionThe approach for testing duplicate transaction rejection is cleaner now, directly verifying the expected error code rather than using custom error matching. The previous approach may have been more fragile.
95-95: Correctly updated timeout flag in backward compatibility testThe backward compatibility test has been properly updated to use the new
--timeout-durationflag, maintaining consistency with the changes in the client module.
| signCmd := []string{"tx", "sign", txFile.Name(), "--from=" + account1Addr, "--chain-id=testing"} | ||
| rsp1 = cli.RunCommandWithArgs(signCmd...) | ||
| signedFile := testutil.TempFile(t) | ||
| signedFile.WriteString(rsp1) | ||
|
|
There was a problem hiding this comment.
Properly implemented transaction signing step
The explicit signing step is correctly implemented using the transaction file and chain ID. Note that line 44 is missing error handling for the file write operation.
Add error handling for the file write operation:
-signedFile.WriteString(rsp1)
+_, err = signedFile.WriteString(rsp1)
+require.NoError(t, err)📝 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.
| signCmd := []string{"tx", "sign", txFile.Name(), "--from=" + account1Addr, "--chain-id=testing"} | |
| rsp1 = cli.RunCommandWithArgs(signCmd...) | |
| signedFile := testutil.TempFile(t) | |
| signedFile.WriteString(rsp1) | |
| signCmd := []string{"tx", "sign", txFile.Name(), "--from=" + account1Addr, "--chain-id=testing"} | |
| rsp1 = cli.RunCommandWithArgs(signCmd...) | |
| signedFile := testutil.TempFile(t) | |
| _, err = signedFile.WriteString(rsp1) | |
| require.NoError(t, err) |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| timeout := clientCtx.Viper.GetDuration(flags.TimeoutDuration) | ||
| var timeoutTimestamp time.Time | ||
| if timeout > 0 { | ||
| timeoutTimestamp = time.Now().Add(timeout) |
Check warning
Code scanning / CodeQL
Calling the system time Warning
aljo242
left a comment
aljo242
left a comment
There was a problem hiding this comment.
are there docs we need to update?
we did not have the flag documented, so i just added some in the recent commit. |
Description
timeout-durationto better reflect the above changeAuthor 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. Your PR will not be merged unless you satisfy
all of these items.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdSummary by CodeRabbit