Skip to content
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

Merged
merged 4 commits into from
Oct 1, 2024

Conversation

Lilyjjo
Copy link
Contributor

@Lilyjjo Lilyjjo commented Sep 30, 2024

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.

@github-actions github-actions bot added the sequencer pertaining to the astria-sequencer crate label Sep 30, 2024
@Lilyjjo Lilyjjo force-pushed the lilyjjo/mempool_transaction_container_test_refactor branch from 0255585 to cd771d6 Compare September 30, 2024 13:46
@Lilyjjo Lilyjjo marked this pull request as ready for review September 30, 2024 13:58
@Lilyjjo Lilyjjo requested a review from a team as a code owner September 30, 2024 13:58
@Lilyjjo Lilyjjo force-pushed the lilyjjo/mempool_transaction_container_test_refactor branch from e018f96 to d09775f Compare September 30, 2024 15:07
@Lilyjjo Lilyjjo force-pushed the lilyjjo/mempool_transaction_container_test_refactor branch from d09775f to cd22f37 Compare October 1, 2024 12:16
Copy link
Member

@SuperFluffy SuperFluffy left a 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:

  1. variable names like ttx_s0_0 are context free and I have no idea why it's called that.
  2. there are signer_0 and signer_1 that get cloned everywhere - why not just use alice or barbara (probably not the right names, but you use the alice key in the builder; just also use alice() instead of signing_key_1.clone().
  3. 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 like nria() -> Denom or usdc() -> Denom or thefunnyasset() -> Denom for which you can make use of the fact that impl From<Denom> for IbcPrefixed, and then for the mock you do my_mock.set_cost(usdc(), 12345)

@@ -779,14 +779,15 @@ impl<const MAX_TX_COUNT: usize> TransactionsContainer<ParkedTransactionsForAccou
}

#[cfg(test)]
mod tests {
mod test {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test is not idiomatic

Suggested change
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 {
Copy link
Member

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)

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,
}
}
Copy link
Member

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(
Copy link
Member

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()
Copy link
Member

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();
Copy link
Member

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

@Lilyjjo
Copy link
Contributor Author

Lilyjjo commented Oct 1, 2024

Since last review:

  • switched back to more explicit mock_tx_cost() call instead of per-denom setter
  • changed mock_tx() into MockTxBuilder::new()
  • switched from hardcoded addresses to use of ALICE\BOB\CAROL

Didn't do:

  • changing use of DENOM_0... or ttx_s1_0_0 into more readable forms. I don't think much value would be gained from switching to something like ttx_signer_alice_nonce_0_variation_0

@Lilyjjo Lilyjjo changed the title refactor(sequencer): use Builder for transaction container tests refactor(sequencer): use builder pattern for transaction container tests Oct 1, 2024
@Lilyjjo Lilyjjo added this pull request to the merge queue Oct 1, 2024
Merged via the queue into main with commit 8e3a474 Oct 1, 2024
46 checks passed
@Lilyjjo Lilyjjo deleted the lilyjjo/mempool_transaction_container_test_refactor branch October 1, 2024 14:48
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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sequencer pertaining to the astria-sequencer crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants