-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(tx)!: make timeout_height time based #20870
Changes from all commits
8d367e6
6de6ec3
cce6b54
6a1fe37
6b142ec
6e2aa27
ae1ca05
59248f5
ce2c7d2
559e0be
a178e9a
02d63dc
efcb0b9
ca0b587
4810fd1
e17ddc8
975c968
2f69f6f
f978027
1013006
c1ba08e
680082e
4c00978
96507c5
d6d2a40
b439a8a
b5b3031
424724b
70f772d
b3a89f5
3724d9d
0fe0792
c74e08f
6edb58e
7b2d554
234d250
4297c62
db37bfa
1657027
55f24fb
9ac9527
e5b2a8e
4323483
0272673
a85d969
f6c0df5
bc94df3
2074128
48c0c5f
4c6c03f
ff273d1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -496,10 +496,10 @@ func (s *ABCIUtilsTestSuite) TestDefaultProposalHandler_NoOpMempoolTxSelection() | |
tx := builder.GetTx() | ||
txBz, err := txConfig.TxEncoder()(tx) | ||
s.Require().NoError(err) | ||
s.Require().Len(txBz, 152) | ||
s.Require().Len(txBz, 165) | ||
|
||
txDataSize := int(cmttypes.ComputeProtoSizeForTxs([]cmttypes.Tx{txBz})) | ||
s.Require().Equal(txDataSize, 155) | ||
s.Require().Equal(txDataSize, 168) | ||
|
||
testCases := map[string]struct { | ||
ctx sdk.Context | ||
|
@@ -532,15 +532,15 @@ func (s *ABCIUtilsTestSuite) TestDefaultProposalHandler_NoOpMempoolTxSelection() | |
Txs: [][]byte{txBz, txBz, txBz, txBz, txBz}, | ||
MaxTxBytes: 465, | ||
}, | ||
expectedTxs: 3, | ||
expectedTxs: 2, | ||
}, | ||
"large max tx bytes len calculation": { | ||
ctx: s.ctx, | ||
req: &abci.PrepareProposalRequest{ | ||
Txs: [][]byte{txBz, txBz, txBz, txBz, txBz}, | ||
MaxTxBytes: 456, | ||
MaxTxBytes: 504, | ||
}, | ||
expectedTxs: 2, | ||
expectedTxs: 3, | ||
}, | ||
"max gas and tx bytes": { | ||
ctx: s.ctx.WithConsensusParams(cmtproto.ConsensusParams{ | ||
|
@@ -619,15 +619,15 @@ func (s *ABCIUtilsTestSuite) TestDefaultProposalHandler_PriorityNonceMempoolTxSe | |
testTxs[i].size = int(cmttypes.ComputeProtoSizeForTxs([]cmttypes.Tx{bz})) | ||
} | ||
|
||
s.Require().Equal(180, testTxs[0].size) | ||
s.Require().Equal(190, testTxs[1].size) | ||
s.Require().Equal(181, testTxs[2].size) | ||
s.Require().Equal(181, testTxs[3].size) | ||
s.Require().Equal(263, testTxs[4].size) | ||
s.Require().Equal(273, testTxs[5].size) | ||
s.Require().Equal(264, testTxs[6].size) | ||
s.Require().Equal(264, testTxs[7].size) | ||
s.Require().Equal(264, testTxs[8].size) | ||
s.Require().Equal(193, testTxs[0].size) | ||
s.Require().Equal(203, testTxs[1].size) | ||
s.Require().Equal(194, testTxs[2].size) | ||
s.Require().Equal(194, testTxs[3].size) | ||
s.Require().Equal(276, testTxs[4].size) | ||
s.Require().Equal(286, testTxs[5].size) | ||
s.Require().Equal(277, testTxs[6].size) | ||
s.Require().Equal(277, testTxs[7].size) | ||
s.Require().Equal(277, testTxs[8].size) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unrelated to this PR but these types of tests are fragile and hard to maintain. |
||
|
||
testCases := map[string]struct { | ||
ctx sdk.Context | ||
|
@@ -640,15 +640,15 @@ func (s *ABCIUtilsTestSuite) TestDefaultProposalHandler_PriorityNonceMempoolTxSe | |
ctx: s.ctx, | ||
txInputs: []testTx{testTxs[0], testTxs[1], testTxs[2], testTxs[3]}, | ||
req: &abci.PrepareProposalRequest{ | ||
MaxTxBytes: 180 + 181, | ||
MaxTxBytes: 193 + 194, | ||
}, | ||
expectedTxs: []int{0, 3}, | ||
}, | ||
"skip multi-signers msg non-sequential sequence": { | ||
ctx: s.ctx, | ||
txInputs: []testTx{testTxs[4], testTxs[5], testTxs[6], testTxs[7], testTxs[8]}, | ||
req: &abci.PrepareProposalRequest{ | ||
MaxTxBytes: 263 + 264, | ||
MaxTxBytes: 276 + 277, | ||
}, | ||
expectedTxs: []int{4, 8}, | ||
}, | ||
|
@@ -657,7 +657,7 @@ func (s *ABCIUtilsTestSuite) TestDefaultProposalHandler_PriorityNonceMempoolTxSe | |
ctx: s.ctx, | ||
txInputs: []testTx{testTxs[9], testTxs[10], testTxs[11]}, | ||
req: &abci.PrepareProposalRequest{ | ||
MaxTxBytes: 263 + 264, | ||
MaxTxBytes: 276 + 277, | ||
}, | ||
expectedTxs: []int{9}, | ||
}, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit, mark timeout height and timeout timestamp mutually exclusive: EDIT: should the height flag even stay?? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we need it for ordered tx ? if not then we can remove the height flag all together There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cc @tac0turtle There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. id remove height flag and just go with time. Its slightly breaking but b etter to push people to the better design There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we also remove There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We shouldn't remove anything from tx body. We can't do that type of breakage in production proto types. We also should still support it in the state machine even if we remove from CLI There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. gotcha There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed the timeout_height flag There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then we do need to flag this as CLI breaking |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
{"body":{"messages":[{"@type":"/cosmos.bank.v1beta1.MsgSend","from_address":"cosmos1y74p8wyy4enfhfn342njve6cjmj5c8dtl6emdk","to_address":"cosmos1y74p8wyy4enfhfn342njve6cjmj5c8dtl6emdk","amount":[{"denom":"foo","amount":"1"}]}]},"auth_info":{"fee":{"gas_limit":"200000"}}} | ||
{"body":{"messages":[{"@type":"/cosmos.bank.v1beta1.MsgSend","from_address":"cosmos1y74p8wyy4enfhfn342njve6cjmj5c8dtl6emdk","to_address":"cosmos1y74p8wyy4enfhfn342njve6cjmj5c8dtl6emdk","amount":[{"denom":"foo","amount":"1"}]}],"timeout_timestamp":"1970-01-01T00:00:00Z"},"auth_info":{"fee":{"gas_limit":"200000"}}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the typo in the function argument.
The argument
unorderedtx.DefaultmaxTimeoutDuration
should beunorderedtx.DefaultMaxTimeoutDuration
as per the standard naming conventions in Go which capitalize each new word in variable names.Committable suggestion