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.

Sign up for GitHub

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

Jump to bottom

[1.x] feat: add invalid-indexes error checking to the advance-runner #523

Merged
merged 3 commits into from
Aug 16, 2024
Merged

[1.x] feat: add invalid-indexes error checking to the advance-runner #523

merged 3 commits into from
Aug 16, 2024

Conversation

renan061
Copy link
Contributor

No description provided.

@renan061 renan061 added the #feat:tests Feature: tests label Jul 18, 2024
@renan061 renan061 added this to the 1.5.0 milestone Jul 18, 2024
@renan061 renan061 self-assigned this Jul 18, 2024
@renan061 renan061 force-pushed the feature/advance-runner-error-checking branch from df15fdf to b1a9dd2 Compare July 18, 2024 04:36
@marcelstanley marcelstanley modified the milestones: 1.5.0, 1.5.1 Jul 26, 2024
@marcelstanley marcelstanley marked this pull request as draft July 26, 2024 14:23
@marcelstanley marcelstanley mentioned this pull request Jul 29, 2024
Closed
2 tasks
@marcelstanley marcelstanley removed this from the 1.5.1 milestone Jul 29, 2024
@marcelstanley marcelstanley changed the title feat: add invalid-indexes error checking to the advance-runner [1.x] feat: add invalid-indexes error checking to the advance-runner Aug 6, 2024
@GMKrieger GMKrieger mentioned this pull request Aug 9, 2024
Closed
@GMKrieger GMKrieger added the no changelog PRs that don't require changes in changelog label Aug 13, 2024
@marcelstanley marcelstanley assigned GMKrieger and unassigned renan061 Aug 13, 2024
@marcelstanley marcelstanley added this to the 1.5.1 milestone Aug 13, 2024
@GMKrieger GMKrieger force-pushed the feature/advance-runner-error-checking branch from b1a9dd2 to 2731d76 Compare August 13, 2024 18:54
@GMKrieger GMKrieger marked this pull request as ready for review August 13, 2024 19:22
@marcelstanley marcelstanley requested a review from a team August 13, 2024 19:24
marcelstanley
marcelstanley reviewed Aug 14, 2024
View reviewed changes
Copy link
Collaborator

@marcelstanley marcelstanley left a comment

Choose a reason for hiding this comment

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

I believe this PR should come after #551, so some rebasing will be needed.

Also, I think we should improve the commit message for 2731d76, explaining in more details what was done.

offchain/advance-runner/src/broker.rs Outdated Show resolved Hide resolved
offchain/rollups-events/src/broker/mod.rs Outdated Show resolved Hide resolved
offchain/rollups-events/tests/integration.rs Outdated Show resolved Hide resolved
offchain/indexer/tests/integration.rs Outdated Show resolved Hide resolved
@marcelstanley marcelstanley requested a review from a team August 14, 2024 01:29
@GMKrieger GMKrieger force-pushed the feature/advance-runner-error-checking branch 2 times, most recently from d0ef7ad to cc79fef Compare August 14, 2024 18:33
@GMKrieger GMKrieger changed the base branch from main to feature/multiapp-claimer August 14, 2024 18:33
marcelstanley
marcelstanley reviewed Aug 14, 2024
View reviewed changes
Copy link
Collaborator

@marcelstanley marcelstanley left a comment

Choose a reason for hiding this comment

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

Can you please change ab96dd6 commit message to feat(advance-runner): detect invalid claim input indexes?

offchain/dispatcher/src/drivers/context.rs Show resolved Hide resolved
@marcelstanley marcelstanley requested a review from a team August 14, 2024 19:16
vfusco
vfusco requested changes Aug 14, 2024
View reviewed changes
offchain/indexer/tests/integration.rs Outdated Show resolved Hide resolved
offchain/dispatcher/src/drivers/context.rs Show resolved Hide resolved
vfusco
vfusco approved these changes Aug 14, 2024
View reviewed changes
offchain/advance-runner/src/broker.rs Outdated Show resolved Hide resolved
vfusco
vfusco requested changes Aug 14, 2024
View reviewed changes
Copy link
Collaborator

@vfusco vfusco left a comment

Choose a reason for hiding this comment

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

Some small changes only

@GMKrieger GMKrieger force-pushed the feature/advance-runner-error-checking branch 2 times, most recently from cf63adf to 390f2e6 Compare August 15, 2024 12:54
vfusco
vfusco previously approved these changes Aug 15, 2024
View reviewed changes
@marcelstanley marcelstanley self-requested a review August 15, 2024 14:39
marcelstanley
marcelstanley previously approved these changes Aug 15, 2024
View reviewed changes
Copy link
Collaborator

@marcelstanley marcelstanley left a comment

Choose a reason for hiding this comment

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

Approved, but I'd rather amending commit fix(dispatcher): check epoch before finishing to

fix(dispatcher): check epoch before finishing

This asserts the input epoch correctness before finish_epoch_if_needed and enqueue_input.

@renan061 renan061 force-pushed the feature/multiapp-claimer branch from 15f40f0 to 55d6233 Compare August 15, 2024 15:50
@GMKrieger GMKrieger force-pushed the feature/advance-runner-error-checking branch 2 times, most recently from eb5767a to 4c421bf Compare August 15, 2024 17:15
@renan061 renan061 force-pushed the feature/multiapp-claimer branch from 55d6233 to b9b9ecf Compare August 15, 2024 17:27
renan061 and others added 3 commits August 16, 2024 10:58
@renan061 @GMKrieger
feat(advance-runner): detect invalid claim indexes
c591d71
@GMKrieger
chore: improve logging
7374587
@GMKrieger
fix(dispatcher): check epoch before finishing
e8d91bc 
This asserts the input epoch correctness before finish_epoch_if_needed and enqueue_input.
@GMKrieger GMKrieger force-pushed the feature/advance-runner-error-checking branch from 4c421bf to e8d91bc Compare August 16, 2024 13:58
@GMKrieger GMKrieger changed the base branch from feature/multiapp-claimer to release/1.5.x August 16, 2024 13:59
@GMKrieger GMKrieger dismissed stale reviews from marcelstanley and vfusco August 16, 2024 13:59

The base branch was changed.

@GMKrieger GMKrieger merged commit e8d91bc into release/1.5.x Aug 16, 2024
5 checks passed
@GMKrieger GMKrieger deleted the feature/advance-runner-error-checking branch August 16, 2024 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcelstanley marcelstanley marcelstanley approved these changes

@vfusco vfusco vfusco approved these changes

Assignees

@GMKrieger GMKrieger

Labels
#feat:tests Feature: tests no changelog PRs that don't require changes in changelog
Projects
Node Unit
Status: ✅ Done
Milestone
1.5.1
Development

Successfully merging this pull request may close these issues.
4 participants
@renan061 @vfusco @marcelstanley @GMKrieger