refactor(sequencer): use builder pattern for transaction container tests#1592
Merged
lobstergrindset merged 4 commits intomainfrom Oct 1, 2024
Merged
Conversation
lobstergrindset
force-pushed
the
lilyjjo/mempool_transaction_container_test_refactor
branch
from
0255585 to
cd771d6
Compare
lobstergrindset
force-pushed
the
lilyjjo/mempool_transaction_container_test_refactor
branch
from
e018f96 to
d09775f
Compare
lobstergrindset
force-pushed
the
lilyjjo/mempool_transaction_container_test_refactor
branch
from
d09775f to
cd22f37
Compare
SuperFluffy
approved these changes
Oct 1, 2024
Contributor
SuperFluffy
left a comment
SuperFluffy
left a comment
There was a problem hiding this comment.
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_0are context free and I have no idea why it's called that. - there are
signer_0andsigner_1that get cloned everywhere - why not just usealiceorbarbara(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_2also 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() -> Denomorusdc() -> Denomorthefunnyasset() -> Denomfor 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)
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| mod test { |
Contributor
There was a problem hiding this comment.
test is not idiomatic
Suggested change
| mod test { | |
| mod tests { |
| const TX_TTL: Duration = Duration::from_secs(2); | ||
|
|
||
| fn mock_ttx( | ||
| struct MockTTXBuilder { |
Contributor
There was a problem hiding this comment.
I am not sure what ttx stands for, but rust eschews ALLCAPS in favor of CamelCase everywhere (even abbreviations)
Suggested change
| struct MockTTXBuilder { | |
| struct MockTtxBuilder { |
But I think you should just call this:
Suggested change
| struct MockTTXBuilder { | |
| struct MockTimemarkedTransaction { |
Comment on lines
824
to
832
| fn default() -> Self { | ||
| Self { | ||
| nonce: 0, | ||
| signer: get_alice_signing_key(), | ||
| denom_0_cost: 0, | ||
| denom_1_cost: 0, | ||
| denom_2_cost: 0, | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
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( |
Contributor
There was a problem hiding this comment.
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() |
Contributor
There was a problem hiding this comment.
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(); |
Contributor
There was a problem hiding this comment.
Why not give the keys names? We have defined a bunch of signers somewhere in test_utils
Contributor
Author
|
Since last review:
Didn't do:
|
lobstergrindset
changed the title
Builder for transaction container tests
lobstergrindset
deleted the
lilyjjo/mempool_transaction_container_test_refactor
branch
steezeburger
added a commit
that referenced
this pull request
Oct 7, 2024
* 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) ...
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.