feat!(tx): make timeout_height time based #20870
Conversation
WalkthroughWalkthroughRecent updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Manager
participant Snapshotter
participant TxWrapper
User->>Manager: Add(txHash, timestamp)
Manager-->>User: Acknowledgment
User->>Snapshotter: RestoreExtension(timestamp, format, pr)
Snapshotter-->>User: Restore Complete
User->>Builder: SetTimeoutTimestamp(timestamp)
Builder-->>User: Timeout Set
User->>TxWrapper: GetTimeoutTimeStamp()
TxWrapper-->>User: Returns time.Time
This diagram visualizes the primary changes and interactions related to managing transactions with timestamps across the system. 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 (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
tac0turtle
changed the title
Co-authored-by: Marko <marko@baricevic.me>
…smos/cosmos-sdk into son/timeout_height_time_based
sontrinh16
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (14)
x/auth/ante/unordered.go (2)
5-5: Add a blank line after imports for better readability.Inserting a blank line after the import statements can improve readability and conform to Go style guidelines.
+
32-34: Ensure consistent naming conventions.The field
maxTimeoutDurationshould be consistently named across the codebase. Consider renamingmaxUnOrderedTTLtomaxTimeoutDurationfor consistency.types/tx_msg.go (1)
7-7: Add a blank line after imports for better readability.Inserting a blank line after the import statements can improve readability and conform to Go style guidelines.
+x/auth/ante/unordered_test.go (2)
6-6: Add a blank line after imports for better readability.Inserting a blank line after the import statements can improve readability and conform to Go style guidelines.
+
10-10: Import only necessary packages.Review if all imported packages are necessary. Remove any unused imports to keep the code clean.
x/auth/ante/unorderedtx/manager.go (4)
37-37: Clarify comment forblockCh.The comment for
blockChshould mention that it now receives timestamps instead of block heights.- // blockCh defines a channel to receive newly committed block heights + // blockCh defines a channel to receive newly committed block timestamps
53-56: Update comment fortxHashes.The comment for
txHashesshould reflect that it now uses timestamps for TTL values.- // txHashes defines a map from tx hash -> TTL value defined as block time, which is used for duplicate + // txHashes defines a map from tx hash -> TTL value defined as timestamp, which is used for duplicate
107-111: UpdateAddfunction parameters in the comment.The comment for
Addshould reflect that it now accepts a timestamp instead of TTL.- // Add adds a new transaction hash with its TTL value + // Add adds a new transaction hash with its timestamp
158-159: ClarifyOnNewBlockfunction comment.The comment for
OnNewBlockshould mention that it now sends the latest block timestamp.- // OnNewBlock sends the latest block number to the background purge loop, which + // OnNewBlock sends the latest block timestamp to the background purge loop, whichclient/tx/aux_builder.go (1)
5-5: Organize imports.Organize the imports to follow the convention of grouping standard library imports separately from third-party imports.
import ( "context" "time" "github.com/cosmos/gogoproto/proto" "google.golang.org/protobuf/types/known/anypb" "google.golang.org/protobuf/types/known/timestamppb" txv1beta1 "cosmossdk.io/api/cosmos/tx/v1beta1" txsigning "cosmossdk.io/x/tx/signing" "cosmossdk.io/x/tx/signing/aminojson" codectypes "github.com/cosmos/cosmos-sdk/codec/types" cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" "github.com/cosmos/cosmos-sdk/types/tx" "github.com/cosmos/cosmos-sdk/types/tx/signing" )client/flags/flags.go (1)
141-141: Clarify flag description.The description for
FlagTimeoutTimestampshould clarify that it expects a Unix timestamp.- f.Int64(FlagTimeoutTimestamp, 0, "Set a block timeout timestamp to prevent the tx from being committed past a certain time") + f.Int64(FlagTimeoutTimestamp, 0, "Set a block timeout timestamp (Unix) to prevent the tx from being committed past a certain time")UPGRADING.md (3)
106-106: Replace hard tabs with spaces.The static analysis tool has flagged the use of hard tabs. Replace them with spaces for consistency.
- ante.NewUnorderedTxDecorator(unorderedtx.DefaultMaxUnOrderedTTL, unorderedtx.DefaultmaxTimeoutDuration, app.UnorderedTxManager), + ante.NewUnorderedTxDecorator(unorderedtx.DefaultMaxUnOrderedTTL, unorderedtx.DefaultmaxTimeoutDuration, app.UnorderedTxManager),Tools
Markdownlint
106-106: Column: 1
Hard tabs(MD010, no-hard-tabs)
103-108: Replace hard tabs with spaces.The static analysis tool has flagged the use of hard tabs. Replace them with spaces for consistency.
- if manager := app.SnapshotManager(); manager != nil { - err := manager.RegisterExtensions(unorderedtx.NewSnapshotter(app.UnorderedTxManager)) - if err != nil { - panic(fmt.Errorf("failed to register snapshot extension: %s", err)) - } - } + if manager := app.SnapshotManager(); manager != nil { + err := manager.RegisterExtensions(unorderedtx.NewSnapshotter(app.UnorderedTxManager)) + if err != nil { + panic(fmt.Errorf("failed to register snapshot extension: %s", err)) + } + }Tools
Markdownlint
103-103: Column: 1
Hard tabs(MD010, no-hard-tabs)
104-104: Column: 1
Hard tabs(MD010, no-hard-tabs)
105-105: Column: 1
Hard tabs(MD010, no-hard-tabs)
106-106: Column: 1
Hard tabs(MD010, no-hard-tabs)
107-107: Column: 1
Hard tabs(MD010, no-hard-tabs)
108-108: Column: 1
Hard tabs(MD010, no-hard-tabs)
106-106: Replace hard tabs with spaces.The static analysis tool has flagged the use of hard tabs. Replace them with spaces for consistency.
- ante.NewUnorderedTxDecorator(unorderedtx.DefaultMaxUnOrderedTTL, unorderedtx.DefaultmaxTimeoutDuration, app.UnorderedTxManager), + ante.NewUnorderedTxDecorator(unorderedtx.DefaultMaxUnOrderedTTL, unorderedtx.DefaultmaxTimeoutDuration, app.UnorderedTxManager),Tools
Markdownlint
106-106: Column: 1
Hard tabs(MD010, no-hard-tabs)
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (1)
types/tx/tx.pb.gois excluded by!**/*.pb.go
Files selected for processing (27)
- CHANGELOG.md (1 hunks)
- UPGRADING.md (1 hunks)
- api/cosmos/tx/v1beta1/tx.pulsar.go (17 hunks)
- baseapp/abci_test.go (1 hunks)
- baseapp/abci_utils_test.go (5 hunks)
- client/flags/flags.go (2 hunks)
- client/tx/aux_builder.go (3 hunks)
- client/tx/factory.go (7 hunks)
- client/tx_config.go (2 hunks)
- client/v2/autocli/testdata/help-echo-msg.golden (1 hunks)
- client/v2/autocli/testdata/msg-output.golden (1 hunks)
- client/v2/offchain/builder.go (1 hunks)
- proto/cosmos/tx/v1beta1/tx.proto (3 hunks)
- simapp/ante.go (1 hunks)
- tests/e2e/baseapp/block_gas_test.go (1 hunks)
- types/errors/errors.go (1 hunks)
- types/tx_msg.go (3 hunks)
- x/auth/ante/basic.go (3 hunks)
- x/auth/ante/basic_test.go (4 hunks)
- x/auth/ante/unordered.go (4 hunks)
- x/auth/ante/unordered_test.go (9 hunks)
- x/auth/ante/unorderedtx/manager.go (12 hunks)
- x/auth/ante/unorderedtx/manager_test.go (5 hunks)
- x/auth/ante/unorderedtx/snapshotter.go (2 hunks)
- x/auth/ante/unorderedtx/snapshotter_test.go (3 hunks)
- x/auth/tx/builder.go (4 hunks)
- x/auth/tx/gogotx.go (2 hunks)
Files not summarized due to errors (1)
- api/cosmos/tx/v1beta1/tx.pulsar.go: Error: Message exceeds token limit
Files skipped from review due to trivial changes (2)
- client/v2/autocli/testdata/help-echo-msg.golden
- client/v2/autocli/testdata/msg-output.golden
Additional context used
Path-based instructions (24)
x/auth/ante/unorderedtx/snapshotter_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"simapp/ante.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.client/tx_config.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/auth/ante/unorderedtx/snapshotter.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/auth/ante/unordered.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/auth/ante/unorderedtx/manager_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"types/tx_msg.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/auth/ante/unordered_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"types/errors/errors.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/auth/ante/unorderedtx/manager.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.client/v2/offchain/builder.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.client/tx/aux_builder.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/auth/tx/gogotx.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tests/e2e/baseapp/block_gas_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/auth/ante/basic.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.client/flags/flags.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/auth/ante/basic_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/auth/tx/builder.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.client/tx/factory.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.baseapp/abci_utils_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"UPGRADING.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"baseapp/abci_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"api/cosmos/tx/v1beta1/tx.pulsar.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Markdownlint
UPGRADING.md
103-103: Column: 1
Hard tabs(MD010, no-hard-tabs)
104-104: Column: 1
Hard tabs(MD010, no-hard-tabs)
105-105: Column: 1
Hard tabs(MD010, no-hard-tabs)
106-106: Column: 1
Hard tabs(MD010, no-hard-tabs)
107-107: Column: 1
Hard tabs(MD010, no-hard-tabs)
108-108: Column: 1
Hard tabs(MD010, no-hard-tabs)
Additional comments not posted (87)
x/auth/ante/unorderedtx/snapshotter_test.go (5)
5-5: Import oftimepackage is appropriate.The
timepackage is required for timestamp calculations in the test function.
16-17: Initialization ofcurrentTimeis correct.Using
time.Now()to initializecurrentTimeis appropriate for the timestamp calculations.
20-20: Addition of unordered transactions with a timestamp is correct.Using
currentTime.Add(time.Second*100)ensures that the transactions have a future timestamp.
42-45: Restoration with a timestamp greater than the timeout time is correct.This test case ensures that no unordered transactions are synced when the timestamp exceeds the timeout time.
49-52: Restoration with a timestamp less than the timeout time is correct.This test case ensures that all unordered transactions are synced when the timestamp is within the timeout time.
simapp/ante.go (1)
42-42: Addition ofNewUnorderedTxDecoratoris correct.This change ensures that unordered transactions with a timeout timestamp are validated.
client/tx_config.go (1)
53-53: Addition ofSetTimeoutTimestampmethod is correct.This change ensures that a timeout timestamp can be set for transactions.
x/auth/ante/unorderedtx/snapshotter.go (5)
7-7: Import oftimepackage is appropriate.The
timepackage is required for timestamp handling in therestorefunction.
13-15: Addition of constants for sizes is correct.These constants improve code readability and maintainability.
82-82: Calculation oftimestampfrom the payload is correct.Using
binary.BigEndian.Uint64ensures that the timestamp is correctly extracted.
83-86: Comment regarding the inclusion of expired transactions is helpful.This comment clarifies the current limitation and the intended behavior.
87-88: Addition of transactions with a timestamp is correct.This change ensures that only valid transactions are added to the manager.
x/auth/ante/unordered.go (2)
53-62: Good validation logic for timeout timestamps.The validation checks for the
timeoutTimestampare well-implemented, ensuring that the timestamp is set, not in the past, and within the allowed duration.
74-74: EnsuretxManager.Addhandles timestamp correctly.Verify that the
Addmethod intxManagercorrectly handles thetimeoutTimestampand maintains the desired behavior for unordered transactions.x/auth/ante/unorderedtx/manager_test.go (5)
28-30: Good use of current time in tests.Using
time.Now()for adding transactions ensures that the tests reflect the new timestamp-based logic accurately.
45-45: Ensure the timestamp is correctly handled inAddmethod.Verify that the
Addmethod correctly handles the timestamp and maintains the desired behavior for unordered transactions in the test cases.
73-73: Ensure persistence of timestamps inAddmethod.Verify that the
Addmethod correctly persists the timestamps and that the transactions are correctly restored in subsequent tests.
103-104: Good use of current time for seeding transactions.Using
currentTimefor seeding transactions ensures that the tests reflect the new timestamp-based logic accurately.
111-113: Ensure accurate time-based purging inOnNewBlockmethod.Verify that the
OnNewBlockmethod correctly purges transactions based on the new timestamp logic and that the tests accurately reflect this behavior.types/tx_msg.go (2)
83-89: NewTxWithTimeoutTimeStampinterface looks good.The new
TxWithTimeoutTimeStampinterface is well-defined and extends theTxinterface appropriately.
102-102: EnsureTxWithUnorderedinterface correctly extendsTxWithTimeoutTimeStamp.Verify that the
TxWithUnorderedinterface correctly extends the newTxWithTimeoutTimeStampinterface and maintains the desired behavior for unordered transactions.x/auth/ante/unordered_test.go (7)
30-33: Good test coverage for ordered transactions.The test case
TestUnorderedTxDecorator_OrderedTxeffectively covers the scenario for ordered transactions, ensuring that the decorator bypasses such transactions correctly.
49-52: Good test coverage for unordered transactions without TTL.The test case
TestUnorderedTxDecorator_UnorderedTx_NoTTLeffectively covers the scenario where an unordered transaction lacks a timeout timestamp, ensuring that an error is returned.
68-71: Good test coverage for unordered transactions with invalid TTL.The test case
TestUnorderedTxDecorator_UnorderedTx_InvalidTTLeffectively covers the scenario where an unordered transaction has an invalid timeout timestamp, ensuring that an error is returned.
86-92: Good test coverage for duplicate unordered transactions.The test case
TestUnorderedTxDecorator_UnorderedTx_AlreadyExistseffectively covers the scenario where an unordered transaction already exists, ensuring that an error is returned.
108-111: Good test coverage for valid unordered transactions in CheckTx mode.The test case
TestUnorderedTxDecorator_UnorderedTx_ValidCheckTxeffectively covers the scenario for valid unordered transactions in CheckTx mode, ensuring that no error is returned.
127-130: Good test coverage for valid unordered transactions in DeliverTx mode.The test case
TestUnorderedTxDecorator_UnorderedTx_ValidDeliverTxeffectively covers the scenario for valid unordered transactions in DeliverTx mode, ensuring that no error is returned and the transaction is added to the manager.
Line range hint
139-157: EnsuregenUnorderedTxhelper function handles timestamps correctly.Verify that the
genUnorderedTxhelper function correctly handles the timestamp and unordered flag, generating transactions that accurately reflect the test scenarios.x/auth/ante/unorderedtx/manager.go (1)
149-150: Ensure correct timestamp conversion.Verify that the conversion from
uint64totime.Timeis correct and consistent with the rest of the codebase.client/v2/offchain/builder.go (1)
116-116: Ensure correct handling ofTimeoutTimestamp.Verify that the
TimeoutTimestampis correctly integrated into the transaction body.client/tx/aux_builder.go (2)
63-69: Ensure correct handling ofTimeoutTimestampinSetTimeoutTimestamp.Verify that the
TimeoutTimestampis correctly set and integrated into the auxiliary signer data.
222-225: Ensure correct handling ofTimeoutTimestampinGetSignBytes.Verify that the
TimeoutTimestampis correctly included in the signing bytes for the transaction.tests/e2e/baseapp/block_gas_test.go (1)
178-178: Document gas consumption calculation.Ensure that the increase in gas consumption is documented for users, as previously discussed.
x/auth/ante/basic.go (1)
231-231: Ensure interface implementation.Ensure that all transactions implementing
TxWithTimeoutHeightalso implement the newGetTimeoutTimeStampmethod.client/flags/flags.go (1)
77-77: Flag definition looks good.The
FlagTimeoutTimestampis correctly defined and integrated.x/auth/ante/basic_test.go (5)
7-7: Importtimepackage.The import of the
timepackage is appropriate for handling timestamp calculations.
197-198: Initialize current time.The initialization of
currentTimeis necessary for the timestamp test cases.
205-220: Add test cases fortimeoutTimestampandtimestamp.The addition of these test cases ensures that the
TxTimeoutHeightDecoratorproperly handles transactions based on both block height and timestamp.
235-235: SettimeoutTimestampin the transaction builder.This line sets the
timeoutTimestampfor the transaction, which is crucial for the new test cases.
242-242: Set block time in the mock header service.This line sets the block time in the mock header service, which is necessary for the timestamp-based test cases.
x/auth/tx/builder.go (6)
5-5: Importtimepackage.The import of the
timepackage is necessary for handling thetimeoutTimestampfield.
9-9: Importtimestamppbpackage.The import of the
timestamppbpackage is necessary for convertingtime.TimetoTimestamp.
83-93: AddtimeoutTimestampfield to builder struct.The addition of the
timeoutTimestampfield to thebuilderstruct is necessary for handling time-based transaction timeouts.
120-120: SettimeoutTimestampinTxBody.The inclusion of the
timeoutTimestampfield in theTxBodyensures that the transaction timeout is based on both block height and timestamp.
195-196: AddSetTimeoutTimestampmethod.The addition of the
SetTimeoutTimestampmethod allows setting thetimeoutTimestampfield in thebuilderstruct.
Line range hint
376-376:
SettimeoutTimestampin transaction.The inclusion of the
timeoutTimestampfield in the transaction ensures that the transaction timeout is based on both block height and timestamp.proto/cosmos/tx/v1beta1/tx.proto (2)
11-11: Importtimestamp.proto.The import of the
timestamp.protofile is necessary for defining thetimeout_timestampfield.
127-134: Addtimeout_timestampfield toTxBodymessage.The addition of the
timeout_timestampfield to theTxBodymessage is necessary for handling time-based transaction timeouts.client/tx/factory.go (7)
9-9: Importtimepackage.The import of the
timepackage is necessary for handling thetimeoutTimestampfield.
37-37: AddtimeoutTimestampfield to Factory struct.The addition of the
timeoutTimestampfield to theFactorystruct is necessary for handling time-based transaction timeouts.
92-93: ParsetimeoutTimestampfrom flags.The addition of the
timeoutTimestampparsing logic ensures that thetimeoutTimestampfield is correctly set from the command-line flags.
112-112: SettimeoutTimestampin Factory.The inclusion of the
timeoutTimestampfield in theFactorystruct ensures that thetimeoutTimestampis correctly set.
143-143: AddTimeoutTimestampmethod.The addition of the
TimeoutTimestampmethod allows retrieving thetimeoutTimestampfield from theFactorystruct.
258-262: AddWithTimeoutTimestampmethod.The addition of the
WithTimeoutTimestampmethod allows setting thetimeoutTimestampfield in theFactorystruct.
376-376: SettimeoutTimestampin transaction.The inclusion of the
timeoutTimestampfield in the transaction ensures that the transaction timeout is based on both block height and timestamp.baseapp/abci_utils_test.go (8)
499-499: LGTM!The code change checks the length of the encoded transaction bytes.
502-502: LGTM!The code change compares the computed size of the transaction data to an expected value.
535-535: LGTM!The code change sets the expected number of transactions based on the
MaxTxBytesvalue.
541-543: LGTM!The code change sets the expected number of transactions based on the
MaxTxBytesvalue.
622-630: LGTM!The code change checks the size of the test transactions.
643-643: LGTM!The code change sets the
MaxTxBytesvalue for thePrepareProposalRequest.
651-651: LGTM!The code change sets the
MaxTxBytesvalue for thePrepareProposalRequest.
660-660: LGTM!The code change sets the
MaxTxBytesvalue for thePrepareProposalRequest.UPGRADING.md (3)
106-106: LGTM!The code correctly adds the unordered transaction decorator to the AnteHandler chain.
Tools
Markdownlint
106-106: Column: 1
Hard tabs(MD010, no-hard-tabs)
103-108: LGTM!The code correctly registers the unordered transaction manager extension with the SnapshotManager.
Tools
Markdownlint
103-103: Column: 1
Hard tabs(MD010, no-hard-tabs)
104-104: Column: 1
Hard tabs(MD010, no-hard-tabs)
105-105: Column: 1
Hard tabs(MD010, no-hard-tabs)
106-106: Column: 1
Hard tabs(MD010, no-hard-tabs)
107-107: Column: 1
Hard tabs(MD010, no-hard-tabs)
108-108: Column: 1
Hard tabs(MD010, no-hard-tabs)
106-106: LGTM!The code correctly adds the unordered transaction decorator to the AnteHandler chain.
Tools
Markdownlint
106-106: Column: 1
Hard tabs(MD010, no-hard-tabs)
baseapp/abci_test.go (2)
1600-1613: Verify transaction byte calculation and insertion logic.Ensure that the logic for calculating
expectedTxBytesand inserting transactions into the mempool is correct.
1617-1622: VerifyPrepareProposalfunction call and response validation.Ensure that the
PrepareProposalfunction is correctly called with theMaxTxBytesparameter and that the response validation checks the expected number of transactions.CHANGELOG.md (2)
200-200: LGTM!The addition correctly communicates the breaking change related to increased gas cost due to the new timeout timestamp field.
206-206: LGTM!The addition correctly communicates the breaking change related to the new
-timeout-timestampflag for tx time-based timeout.api/cosmos/tx/v1beta1/tx.pulsar.go (17)
17-17: Import addition aligns with new field usage.The addition of
timestamppbis necessary for handling the newtimeout_timestampfield.
2772-2772: Field descriptor addition aligns with new proto field.The addition of
fd_TxBody_timeout_timestampis necessary for the newtimeout_timestampfield inTxBody.
2784-2784: Field initialization aligns with new field descriptor.The initialization of
fd_TxBody_timeout_timestampis necessary for the newtimeout_timestampfield inTxBody.
2878-2883: Proto reflection handling for new field.The added handling for
timeout_timestampensures the new field is correctly managed in proto reflection operations.
2919-2920: Existence check for new proto field.The check for
timeout_timestampensures the presence of the new field is correctly identified.
2949-2950: Reset handling for new proto field.The reset handling for
timeout_timestampensures the new field can be correctly reset.
2986-2988: Value retrieval for new proto field.The retrieval of
timeout_timestampvalue ensures the new field can be correctly accessed in proto reflection.
3031-3032: Assignment handling for new proto field.The assignment for
timeout_timestampensures the new field can be correctly set.
3068-3071: Default initialization for new proto field.The default initialization for
timeout_timestampensures the new field is correctly initialized when absent.
3112-3114: Default value retrieval for new proto field.The default value retrieval for
timeout_timestampensures the new field can provide a default value when required.
3206-3209: Size calculation for new proto field.The size calculation for
timeout_timestampensures the new field's size is correctly accounted for in proto operations.
3287-3300: Marshaling for new proto field.The marshaling for
timeout_timestampensures the new field can be correctly serialized in proto operations.
3493-3527: Unmarshaling for new proto field.The unmarshaling for
timeout_timestampensures the new field can be correctly deserialized in proto operations.
8561-8571: Documentation for new proto field.The documentation for
timeout_timestampensures the new field is correctly described in the proto definition.
8630-8635: Getter method for new proto field.The getter method for
timeout_timestampensures the new field can be correctly accessed in the code.
9152-9319: Type definition for new proto field.The type definition for
timeout_timestampensures the new field's type is correctly defined in the proto file.
9350-9350: Dependency addition for new proto field.The addition of the
timestamppb.Timestampdependency ensures the new field's type is correctly referenced in the proto file.
|
|
||
| // ErrTxTimeout defines an error for when a tx is rejected out due to an | ||
| // explicitly set timeout timestamp. | ||
| ErrTxTimeout = errorsmod.Register(RootCodespace, 42, "tx timeout") |
There was a problem hiding this comment.
Add documentation for ErrTxTimeout.
The new error constant ErrTxTimeout is added correctly. However, it lacks documentation. Ensure it follows the existing pattern of documenting errors.
// ErrTxTimeout defines an error for when a tx is rejected due to an
// explicitly set timeout timestamp.
ErrTxTimeout = errorsmod.Register(RootCodespace, 42, "tx timeout")| // GetTimeoutHeight returns the transaction's timeout height (if set). | ||
| func (w *gogoTxWrapper) GetTimeoutTimeStamp() time.Time { | ||
| return w.Tx.Body.TimeoutTimestamp.AsTime() | ||
| } |
There was a problem hiding this comment.
Ensure method name consistency.
The comment for GetTimeoutTimeStamp incorrectly refers to the method as GetTimeoutHeight. Please update the comment to match the method name.
- // GetTimeoutHeight returns the transaction's timeout height (if set).
+ // GetTimeoutTimeStamp returns the transaction's timeout timestamp (if set).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.
| // GetTimeoutHeight returns the transaction's timeout height (if set). | |
| func (w *gogoTxWrapper) GetTimeoutTimeStamp() time.Time { | |
| return w.Tx.Body.TimeoutTimestamp.AsTime() | |
| } | |
| // GetTimeoutTimeStamp returns the transaction's timeout timestamp (if set). | |
| func (w *gogoTxWrapper) GetTimeoutTimeStamp() time.Time { | |
| return w.Tx.Body.TimeoutTimestamp.AsTime() | |
| } |
| func (m *Manager) expiredTxs(blockTime time.Time) []TxHash { | ||
| m.mu.RLock() | ||
| defer m.mu.RUnlock() | ||
|
|
||
| var result []TxHash | ||
| for txHash, ttl := range m.txHashes { | ||
| if blockHeight > ttl { | ||
| for txHash, timestamp := range m.txHashes { | ||
| if blockTime.After(timestamp) { |
There was a problem hiding this comment.
Optimize expired transaction check.
Consider using a more efficient way to check for expired transactions, especially if the map is large.
func (m *Manager) expiredTxs(blockTime time.Time) []TxHash {
m.mu.RLock()
defer m.mu.RUnlock()
var result []TxHash
for txHash, timestamp := range m.txHashes {
if blockTime.After(timestamp) {
result = append(result, txHash)
}
}
return result
}| // DefaultmaxTimeoutDuration defines the default maximum duration an un-ordered transaction | ||
| // can set. | ||
| DefaultMaxUnOrderedTTL = 1024 | ||
| // TODO: need to decide a default value | ||
| DefaultmaxTimeoutDuration = time.Minute * 40 |
There was a problem hiding this comment.
Typographical error in constant name.
The constant DefaultmaxTimeoutDuration should be renamed to follow Go naming conventions.
- DefaultmaxTimeoutDuration = time.Minute * 40
+ DefaultMaxTimeoutDuration = time.Minute * 40Committable 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.
| // DefaultmaxTimeoutDuration defines the default maximum duration an un-ordered transaction | |
| // can set. | |
| DefaultMaxUnOrderedTTL = 1024 | |
| // TODO: need to decide a default value | |
| DefaultmaxTimeoutDuration = time.Minute * 40 | |
| // DefaultmaxTimeoutDuration defines the default maximum duration an un-ordered transaction | |
| // can set. | |
| // TODO: need to decide a default value | |
| DefaultMaxTimeoutDuration = time.Minute * 40 |
| timeoutTimestamp := timeoutTx.GetTimeoutTimeStamp() | ||
| if !timeoutTimestamp.IsZero() && timeoutTimestamp.Unix() != 0 && timeoutTimestamp.Before(headerInfo.Time) { | ||
| return errorsmod.Wrapf( | ||
| sdkerrors.ErrTxTimeout, "block time: %s, timeout timestamp: %s", headerInfo.Time.String(), timeoutTimestamp.String(), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Check for zero timestamp.
The check for timeoutTimestamp.IsZero() is redundant since timeoutTimestamp.Unix() != 0 already covers it. Consider simplifying the condition.
- if !timeoutTimestamp.IsZero() && timeoutTimestamp.Unix() != 0 && timeoutTimestamp.Before(headerInfo.Time) {
+ if timeoutTimestamp.Unix() != 0 && timeoutTimestamp.Before(headerInfo.Time) {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 := timeoutTx.GetTimeoutTimeStamp() | |
| if !timeoutTimestamp.IsZero() && timeoutTimestamp.Unix() != 0 && timeoutTimestamp.Before(headerInfo.Time) { | |
| return errorsmod.Wrapf( | |
| sdkerrors.ErrTxTimeout, "block time: %s, timeout timestamp: %s", headerInfo.Time.String(), timeoutTimestamp.String(), | |
| ) | |
| } | |
| timeoutTimestamp := timeoutTx.GetTimeoutTimeStamp() | |
| if timeoutTimestamp.Unix() != 0 && timeoutTimestamp.Before(headerInfo.Time) { | |
| return errorsmod.Wrapf( | |
| sdkerrors.ErrTxTimeout, "block time: %s, timeout timestamp: %s", headerInfo.Time.String(), timeoutTimestamp.String(), | |
| ) | |
| } |
Description
ref: 20658
txbodyTxTimeoutHeightDecoratorandUnorderedTxDecoratorvalidateTx check accounts for both timeout height and timeout timestampAuthor 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
Bug Fixes
Documentation