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

tests: ✨ provide app state in genesis request #3810

Merged
merged 2 commits into from
Feb 13, 2024

Conversation

cratelyn
Copy link
Contributor

this is a follow-on to #3807. this makes the
mock_consensus_can_send_an_init_chain_request test pass.

see #3588.

add an app state field to the builder, thread that through to the abci request. log this after the response is received.

; RUST_LOG="penumbra-app=debug,info" cargo test --package penumbra-app init_chain -- --nocapture

running 1 test
  2024-02-13T15:18:01.770473Z  INFO cnidarium::storage: snapshot dispatcher task has started
    at crates/cnidarium/src/storage.rs:208

  2024-02-13T15:18:01.880291Z  INFO penumbra_app::server::consensus: genesis state is a full configuration
    at crates/core/app/src/server/consensus.rs:143

  2024-02-13T15:18:01.883007Z  INFO penumbra_app::server::consensus: finished init_chain, consensus_params: Params { block: Size { max_bytes: 1, max_gas: 1, time_iota_ms: 1 }, evidence: Params { max_age_num_blocks: 1, max_age_duration: Duration(1s), max_bytes: 1 }, validator: ValidatorParams { pub_key_types: [] }, version: Some(VersionParams { app: 1 }), abci: AbciParams { vote_extensions_enable_height: None } }, validators: [], app_hash: RootHash("5fc87d8a020365f0bc2cff7b1e8fc5dc3d14f862a7c860dc231e6d44fa847277")
    at crates/core/app/src/server/consensus.rs:153

  2024-02-13T15:18:01.883067Z  INFO mock_consensus: finished init chain, hash: [5F, C8, 7D, 8A, 2, 3, 65, F0, BC, 2C, FF, 7B, 1E, 8F, C5, DC, 3D, 14, F8, 62, A7, C8, 60, DC, 23, 1E, 6D, 44, FA, 84, 72, 77]
    at crates/core/app/tests/mock_consensus.rs:30

test mock_consensus_can_send_an_init_chain_request ... ok

this is a follow-on to #3807. this makes the
`mock_consensus_can_send_an_init_chain_request` test pass.

see #3588.

add an app state field to the builder, thread that through to the abci
request. log this after the response is received.
@cratelyn cratelyn added the A-mock-consensus Area: Relates to the mock consensus engine label Feb 13, 2024
@cratelyn cratelyn self-assigned this Feb 13, 2024
@cratelyn cratelyn marked this pull request as ready for review February 13, 2024 15:30
@cratelyn cratelyn requested a review from hdevalence February 13, 2024 15:30
Copy link
Member

@hdevalence hdevalence left a comment

Choose a reason for hiding this comment

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

We should be careful to keep the mock engine API penumbra-agnostic and keep the penumbra specific parts in the extension trait we scoped out while pairing.

crates/test/mock-consensus/src/builder.rs Outdated Show resolved Hide resolved
cratelyn added a commit that referenced this pull request Feb 13, 2024
rephrase the builder interfaces in terms of application-agnostic types.

addresses the pr review linked below.

NB: because `InitChain::app_state_bytes` is a `Bytes`, see
https://rustdoc.penumbra.zone/main/tendermint/abci/request/struct.InitChain.html#structfield.app_state_bytes
...i've written `Builder::app_state` to accept an `Into<Bytes>`. this
allows callers to provide some other common types, like...

- https://docs.rs/bytes/latest/bytes/struct.Bytes.html#impl-From%3C%26'static+%5Bu8%5D%3E-for-Bytes
- https://docs.rs/bytes/latest/bytes/struct.Bytes.html#impl-From%3C%26'static+str%3E-for-Bytes
- https://docs.rs/bytes/latest/bytes/struct.Bytes.html#impl-From%3CBox%3C%5Bu8%5D,+Global%3E%3E-for-Bytes
- https://docs.rs/bytes/latest/bytes/struct.Bytes.html#impl-From%3CString%3E-for-Bytes

> This should be taking a Vec, so that the mock engine code doesn't
> build in Penumbra dependencies. Instead, we'll want to have a
> penumbra_app_state method in an extension trait like we sketched while
> pairing that does the serialization and calls this method with the
> resulting bytes.

#3810 (review)

referenced code: https://github.com/penumbra-zone/penumbra/pull/3597/files#diff-4b1c924f57b5a531a0be51a9975a35280b53ba9c3f8019a01839859a171aa9a1R10

Co-Authored-By: Henry de Valence <[email protected]>
rephrase the builder interfaces in terms of application-agnostic types.

addresses the pr review linked below.

NB: because `InitChain::app_state_bytes` is a `Bytes`, see
https://rustdoc.penumbra.zone/main/tendermint/abci/request/struct.InitChain.html#structfield.app_state_bytes
...i've written `Builder::app_state` to accept an `Into<Bytes>`. this
allows callers to provide some other common types, like...

- https://docs.rs/bytes/latest/bytes/struct.Bytes.html#impl-From%3C%26'static+%5Bu8%5D%3E-for-Bytes
- https://docs.rs/bytes/latest/bytes/struct.Bytes.html#impl-From%3C%26'static+str%3E-for-Bytes
- https://docs.rs/bytes/latest/bytes/struct.Bytes.html#impl-From%3CBox%3C%5Bu8%5D,+Global%3E%3E-for-Bytes
- https://docs.rs/bytes/latest/bytes/struct.Bytes.html#impl-From%3CString%3E-for-Bytes

> This should be taking a Vec, so that the mock engine code doesn't
> build in Penumbra dependencies. Instead, we'll want to have a
> penumbra_app_state method in an extension trait like we sketched while
> pairing that does the serialization and calls this method with the
> resulting bytes.

#3810 (review)

referenced code: https://github.com/penumbra-zone/penumbra/pull/3597/files#diff-4b1c924f57b5a531a0be51a9975a35280b53ba9c3f8019a01839859a171aa9a1R10

Co-Authored-By: Henry de Valence <[email protected]>
@cratelyn cratelyn force-pushed the katie/send-init-chain-contd branch from 17cfd12 to 0c201c8 Compare February 13, 2024 17:19
@cratelyn cratelyn merged commit 7d61991 into main Feb 13, 2024
7 checks passed
@cratelyn cratelyn deleted the katie/send-init-chain-contd branch February 13, 2024 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-mock-consensus Area: Relates to the mock consensus engine
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants