-
Notifications
You must be signed in to change notification settings - Fork 302
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
app: 💸 a mock consensus spend is performed #3891
app: 💸 a mock consensus spend is performed #3891
Conversation
this makes a small (no-op) change to `fmd_precision_within_grace_period`. this will provide an event that can be used to identify why a clue did not satisfy the consensus rules. using tracing's pretty formatter: ``` 2024-02-26T21:26:49.825156Z ERROR penumbra_app::action_handler::transaction::stateful: invalid clue precision, clue_precision: 1, using_current_precision: false, using_previous_precision: false, within_grace_pe riod: true at crates/core/app/src/action_handler/transaction/stateful.rs:62 in penumbra_app::action_handler::transaction::stateful::fmd_precision_within_grace_period with current_fmd.precision_bits: 0, previous_fmd.precision_bits: 0, previous_fmd.as_of_block_height: 1 in penumbra_app::action_handler::transaction::check_stateful in penumbra_mock_consensus::abci::deliver_tx in penumbra_mock_consensus::block::execute with height: 1, time: 1708982809 ```
af63e0b
to
0269234
Compare
// Build a transaction spending this note. | ||
let tx: Transaction = { |
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.
a passing thought, but this part of the API is pretty long-winded. we talked a bit about this in our sprint planning meeting today, but i'd love to see a more convenient way to go about this for future test authors, without introducing a third planner API.
that is a design conversation that shouldn't block this PR, though.
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.
The API we'd like to use is the Planner
in the view crate. We'd need to be able to pass the MockClient
to its plan() method: https://rustdoc.penumbra.zone/main/penumbra_view/struct.Planner.html#method.plan
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.
x-ref #3896, i think?
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.
Clarification post 54c0b35 , where the transaction plan creation becomes much simpler: my point is that for simple tests, where we can write down the complete transaction plan explicitly, we should do so. In this case, we can, which is what 54c0b35 does.
If we need to do more complex test logic, test logic that is complex enough that we cannot simply write down the plan, then we should decide on a way to use the Planner
API to plan transactions, rather than have another set of helper methods for generating plans. This may involve pulling in the full rust view server implementation, or continuing to use the MockClient
. But for simple cases it's important to be able to test the chain logic without all the complexity of the rust view server.
This commit moves some common functionality into the MockClient itself, in order to simplify the layout of the test logic. Also, we add explicit invariant checks on the chain state.
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.
This looks great! I think it would be good to deduplicate a bit with the tests we already have, after we get this merged.
fixes #3788.
this branch builds upon the work in #3857 and #3866, filling out the remaining holes in the transaction plan. this now performs a spend, using the mock consensus engine.