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

feat(app): 🎋 add missing Sct component hooks #4246

Merged
merged 1 commit into from
Apr 19, 2024

Conversation

cratelyn
Copy link
Contributor

@cratelyn cratelyn commented Apr 19, 2024

fixes #3739.

the App only ever invokes the Sct component's init_chain and begin_block hooks.

this isn't currently a bug, since its end_block and end_epoch hooks are noöps, but could easily lead to a surprising outcome in the future, if more logic was introduced to those trait methods.

checklist before requesting a review

  • if this code contains consensus-breaking changes, i have added the "consensus-breaking" label. otherwise, i declare my belief that there are not consensus-breaking changes, for the following reason:

    while this does change the control flow of consensus-critical logic, this does not
    change the execution of incoming data. the hooks introduced here are noöps.

fixes #3799.

the `App` only ever invokes the Sct component's `init_chain` and
`begin_block` hooks.

this isn't currently a bug, since its `end_block` and `end_epoch` hooks
are noöps, but could easily lead to a surprising outcome in the future,
if more logic was introduced to those trait methods.
@cratelyn cratelyn added A-node Area: System design and implementation for node software E-easy Effort: Easy _P-V1 Priority: slated for V1 release _P-low Priority: low labels Apr 19, 2024
@cratelyn cratelyn added this to the Sprint 4 milestone Apr 19, 2024
@cratelyn cratelyn self-assigned this Apr 19, 2024
@cratelyn cratelyn changed the title feat(app): 🎋 add missing Sct component hooks feat(app): 🎋 add missing Sct component hooks Apr 19, 2024
@cratelyn
Copy link
Contributor Author

cratelyn commented Apr 19, 2024

#[instrument(name = "sct_component", skip(_state, _end_block))]
async fn end_block<S: StateWrite + 'static>(
_state: &mut Arc<S>,
_end_block: &abci::request::EndBlock,
) {
}
#[instrument(name = "sct_component", skip(_state))]
async fn end_epoch<S: StateWrite + 'static>(_state: &mut Arc<S>) -> anyhow::Result<()> {
Ok(())
}

for reference:

cratelyn added a commit that referenced this pull request Apr 19, 2024
see #3799, and #4246.

while wiring up a pair of missing component hooks for the `Sct`
component, i found another (noöp) hook that we do not include in the
`App`'s control flow.

this isn't currently a bug, since `<Dex as Component>::begin_block()`
is a noöps but could easily lead to a surprising outcome in the future, if
more logic was introduced to that trait method.
@cratelyn cratelyn marked this pull request as ready for review April 19, 2024 17:39
@cratelyn cratelyn merged commit fae3d5f into main Apr 19, 2024
8 checks passed
@cratelyn cratelyn deleted the kate/add-missing-sct-component-hooks branch April 19, 2024 18:26
cratelyn added a commit that referenced this pull request Apr 19, 2024
see #3739, and #4246.

while wiring up a pair of missing component hooks for the `Sct`
component, i found another (noöp) hook that we do not include in the
`App`'s control flow.

this isn't currently a bug, since `<Dex as Component>::begin_block()` is
a noöps but could easily lead to a surprising outcome in the future, if
more logic was introduced to that trait method.

#### checklist before requesting a review

- [x] if this code contains consensus-breaking changes, i have added the
"consensus-breaking" label. otherwise, i declare my belief that there
are not consensus-breaking changes, for the following reason:

> while this **does** change the control flow of consensus-critical
logic, this does **not**
change the execution of incoming data. the hooks introduced here are
noöps.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-node Area: System design and implementation for node software E-easy Effort: Easy _P-low Priority: low _P-V1 Priority: slated for V1 release
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

app: 🎋 missing Sct component hooks
2 participants