-
Notifications
You must be signed in to change notification settings - Fork 75
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
refactor(sequencer): use builder pattern for transaction container tests #1592
refactor(sequencer): use builder pattern for transaction container tests #1592
Conversation
0255585
to
cd771d6
Compare
e018f96
to
d09775f
Compare
d09775f
to
cd22f37
Compare
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.
I am fine with the changes (minus the conventions on the naming, see my comment).
Because this is the first time I am reading these tests - my problem with the tests is not necessarily how you construct mocked transactions (although this builder is certainly an improvement), but that I don't really understand what's going on. For example:
- variable names like
ttx_s0_0
are context free and I have no idea why it's called that. - there are
signer_0
andsigner_1
that get cloned everywhere - why not just usealice
orbarbara
(probably not the right names, but you use the alice key in the builder; just also usealice()
instead ofsigning_key_1.clone()
. denom_0
,denom_1
,denom_2
also made no sense to me inially. I needed to read the code to understand what's being tested. IMO stay closer to your actual impl and just provide ways to set the actual objects in the map (concerns around safety and panicking don't matter in tests, so we can make those very readable). For the assets, similar to the keys before, I would also provide constants likenria() -> Denom
orusdc() -> Denom
orthefunnyasset() -> Denom
for which you can make use of the fact thatimpl From<Denom> for IbcPrefixed
, and then for the mock you domy_mock.set_cost(usdc(), 12345)
@@ -779,14 +779,15 @@ impl<const MAX_TX_COUNT: usize> TransactionsContainer<ParkedTransactionsForAccou | |||
} | |||
|
|||
#[cfg(test)] | |||
mod tests { | |||
mod test { |
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.
test
is not idiomatic
mod test { | |
mod tests { |
@@ -798,17 +799,72 @@ mod tests { | |||
const MAX_PARKED_TXS_PER_ACCOUNT: usize = 15; | |||
const TX_TTL: Duration = Duration::from_secs(2); | |||
|
|||
fn mock_ttx( | |||
struct MockTTXBuilder { |
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.
I am not sure what ttx stands for, but rust eschews ALLCAPS in favor of CamelCase everywhere (even abbreviations)
struct MockTTXBuilder { | |
struct MockTtxBuilder { |
But I think you should just call this:
struct MockTTXBuilder { | |
struct MockTimemarkedTransaction { |
fn default() -> Self { | ||
Self { | ||
nonce: 0, | ||
signer: get_alice_signing_key(), | ||
denom_0_cost: 0, | ||
denom_1_cost: 0, | ||
denom_2_cost: 0, | ||
} | ||
} |
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.
I think this can be folded into new
.signer(signing_key_0.clone()) | ||
.build(); | ||
// Same nonce and signer as `ttx_s0_0_0`, but different chain id. | ||
let ttx_s0_0_1 = TimemarkedTransaction::new( |
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.
Why does this not use the same MockTTXBuilder
? Personally, that's more confusing to me than anything. Can you just add the chain-id
as a field that's also set to a default, like the signer?
TimemarkedTransaction::new(mock_tx(0, &signing_key_0, "other"), mock_tx_cost(0, 0, 0)); | ||
let ttx_s0_2_0 = mock_ttx(2, &signing_key_0, 0, 0, 0); | ||
let ttx_s1_0_0 = mock_ttx(0, &signing_key_1, 0, 0, 0); | ||
let ttx_s0_0_0 = MockTTXBuilder::new() |
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.
As a reader, these names are impossible to understand and hard to track.
.nonce(0) | ||
.signer(signing_key_0.clone()) | ||
.build(); | ||
let ttx_s0_1 = MockTTXBuilder::new().nonce(1).signer(signing_key_0).build(); |
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.
Why not give the keys names? We have defined a bunch of signers somewhere in test_utils
Since last review:
Didn't do:
|
Builder
for transaction container tests * main: (34 commits) feat(proto): add bundle and optimistic block apis (#1519) feat(sequencer)!: make empty transactions invalid (#1609) chore(sequencer): simplify boolean expressions in `transaction container` (#1595) refactor(cli): merge argument parsing and command execution (#1568) feat(charts): astrotrek chart (#1513) chore(charts): genesis template to support latest changes (#1594) fix(ci): code freeze action fix (#1610) chore(sequencer)!: exclusively use Borsh encoding for stored data (ENG-768) (#1492) ci: code freeze through github actions (#1588) refactor(sequencer): use builder pattern for transaction container tests (#1592) feat(conductor)!: implement chain ID checks (#1482) chore(ci): upgrade audit-check (#1577) feat(sequencer)!: transaction categories on UnsignedTransaction (#1512) fix(charts): sequencer prometheus rules (#1598) chore(all): Migrate all instances of `#[allow]` to `#[expect]` (#1561) chore(charts,sequencer-faucet): asset precision (#1517) chore(docs): endpoints (#1543) fix(docker): use target binary build param as name of image entrypoint (#1573) fix(ci): ibc bridge test timeout (#1587) Feature: Add `graph-node` to charts. (#1489) ...
Summary
Switched the method of constructing transactions from a constructor to a builder pattern.
Background
We're about to add more relevant transaction fields (ActionGroup type, valid block number), and this change will make it possible to add these extra fields without having to change all of the other tests.
Testing
The changes are contained to the unit tests to
crates/astria-sequencer/mempool/transaction_container.rs
. All unit tests still pass.