Skip to content

Conversation

AndriiDiachuk
Copy link
Contributor

closes: #7656

Context

  • Refactored access and observer builders to initialize newly added members before creating snapshot mock.
  • Updated backend endpoint (GetExecutionDataByBlockID).
  • Added arguments to backend implementation to specify user query criteria.
  • Updated test according to changes.

…a to snapshot interface, refactored access and observer node builders, fixed tests
@AndriiDiachuk AndriiDiachuk self-assigned this Sep 30, 2025
Copy link
Contributor

@peterargue peterargue left a comment

Choose a reason for hiding this comment

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

added a few small comments. overall this looks good. will wait for the rest of your changes before doing a thorough review

Base automatically changed from illia-malachyn/7652-fork-aware-events-endpoint to feature/optimistic-sync October 6, 2025 10:44
}

// TODO: use real objects instead of mocks once they're implemented
snapshot := osyncsnapshot.NewSnapshotMock(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn’t add the notNil() check for any of the storages because some integration tests fail when it’s added. It’s expected that these storages can be nil at that point, they’re either initialized or being initialized later during module setup, depending on flags such as storeTxResultErrorMessages and executionDataSyncEnabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

If any of these storages are not set when this module is run, then the snapshot will store nil for the db. Then when the API requests the db, it will get nil and panic.

I think adding the notNil() check is a good idea. if anything is nil during bootstrapping, that points to an ordering problem that needs to be fixed.

Copy link
Contributor

@UlyanaAndrukhiv UlyanaAndrukhiv left a comment

Choose a reason for hiding this comment

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

Adding a few small comments, but otherwise this looks good to me. I also agree with Peter's outstanding comments.

Copy link
Contributor

@peterargue peterargue left a comment

Choose a reason for hiding this comment

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

mostly small comments. the main 2 were about error handling and bootstrapping. otherwise this looks good

.mockery.yaml Outdated
interfaces:
Core:
Snapshot:
BlockExecutionDataReader:
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens if we remove all of the config for github.com/onflow/flow-go/module/executiondatasync/optimistic_sync and just let it generate all of the mocks? if it's only a couple extra we may not need, I vote we do that to keep the config simple

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@peterargue yes, it will generate additional mocks for(PipelineStateConsumer, PipelineFactory, PipelineStateProvider, CoreFactory), which I believe is not too much. So I will remove the separate config for optimistic_sync.

}

// TODO: use real objects instead of mocks once they're implemented
snapshot := osyncsnapshot.NewSnapshotMock(
Copy link
Contributor

Choose a reason for hiding this comment

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

If any of these storages are not set when this module is run, then the snapshot will store nil for the db. Then when the API requests the db, it will get nil and panic.

I think adding the notNil() check is a good idea. if anything is nil during bootstrapping, that points to an ordering problem that needs to be fixed.

Copy link
Contributor

@UlyanaAndrukhiv UlyanaAndrukhiv left a comment

Choose a reason for hiding this comment

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

Looks good to me! I left a few minor comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants