Skip to content

[Access] Get Block endpoint is missing the system collection #514

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
Merged
merged 3 commits into from
Nov 29, 2023
Merged

[Access] Get Block endpoint is missing the system collection #514

merged 3 commits into from
Nov 29, 2023

Conversation

AndriiDiachuk
Copy link
Collaborator

onflow/flow-go#4584

This pull request implements two missing methods GetSystemTransaction, GetSystemTransactionResult from Access API interface.

@AndriiDiachuk AndriiDiachuk changed the title Added missing methods for emulator [Access] Get Block endpoint is missing the system collection Nov 27, 2023
@AndriiDiachuk AndriiDiachuk mentioned this pull request Nov 27, 2023
Merged
devbugging
devbugging suggested changes Nov 27, 2023
View reviewed changes
Copy link
Contributor

@devbugging devbugging left a comment

Choose a reason for hiding this comment

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

Let's wait for that PR to get merged

jordanschalm added a commit to onflow/flow-go that referenced this pull request Nov 28, 2023
@jordanschalm
modifies the flow-emulator version pinned to match replace statement
2683732 
See:
- onflow/flow-emulator#514
- #5049
@jordanschalm
Copy link
Member

Let's wait for that PR to get merged

As far as I can tell, this change is backward-compatible on the flow-emulator side and not backward-compatible on the flow-go side. In these cases, would it not make sense to merge the flow-emulator PR first?

jordanschalm added a commit to onflow/flow-go that referenced this pull request Nov 28, 2023
@jordanschalm @ramtinms
@turbolent @Guitarheroua @janezpodhostnik @thep2p @kc1116 @zhangchiqing @devbugging @j1010001 @koko1123 @peterargue @AndriiDiachuk @UlyanaAndrukhiv @durkmurder @Kay-Zee
Sync master / FLIP 204 (#5065)
0b64668 
* define direct call types

* update docs

* remove unnecessary event types

* .

* add more contract types for testing

* fix tests

* skip test, requires funding (implemented in follow up PR)

* unskip test

* check error

* • extended compile-time checks for telemetry consumer enforcing that all happy-path interfaces are implemented
• extended subscriptions of telemetry consumer to receive missing events

* reduced notifications consumer interface in PaceMaker, as it only emits events from `hotstuff.ParticipantConsumer`

* update tests

* Refactor event emmision code

* moves fixtures from insecure to p2ptest

* adds gossipsub message fixture function

* refactors test fixtures

* creates GossipSubRpcFixture

* adds documentation

* adds godoc

* Update network/p2p/unicast/README.MD

Co-authored-by: Khalil Claybon <[email protected]>

* expose more test utility

* lint

* add register store

* update LastFinalizedAndExecutedHeight

* convert storage.ErrNotFound to nil

* fix lint

* fixes the logger

* moving ready to the select-case

* moving ready to select-case

* revises the error by update loop to make it irrecoverable

* clean up benchmarking

* adds select case for startup of subscription provider

* Update network/p2p/scoring/subscription_provider.go

Co-authored-by: Khalil Claybon <[email protected]>

* adds documentation to subscription record cache

* fixes a bug with subscription record id

* adds more documentation for current cycle

* untangled Register API handler

* fix epoch event docs

* chnages per suggestions

* remove mock

* [Access] Handle script canceled and timeout errors

* [Access] Make script exec configurable

* Make computation limit configurable on ENs

* set limit setting once

* undo whitespace changes

* add storehouse loader

* add loaders

* add comments update log

* add comments

* update loader

* add extending block snapshot

* fix tests

* fix lint

* update interface

* update tests

* fix lint

* update committer

* update mocks

* update committer tests

* refactor collector

* update mocks

* update bootstrap

* fix noop committer

* fix computer_test

* fix tests

* refactor test

* update committer tests

* fix tests

* Changed input parameters for new methods

* Updated according to comments

* Update engine/access/rpc/backend/backend_scripts_test.go

Co-authored-by: Gregor G. <[email protected]>

* fix committer tests

* add comment

* update tests

* Added implementation for execution api engine

* Added tests for execution api engine

* Added implementation for method for returning tx result

* Fixed issues based on comments

* Added accidentally removed code

* Updated implementations following protobuf changes,updated mocks

* Linted

* [Access] Update websockets event streaming to return JSON-CDC encoded events

* Added tests for sentinel errors

* Update engine/execution/rpc/engine.go

Co-authored-by: Peter Argue <[email protected]>

* Generated mocks, added missing argument to method:

* fix per suggestion

* cleanup

* fix mocks

* lint

* update NewStorageSnapshot

* update mocks

* update tests

* refactor script engine

* update mocks

* fix tests

* fix tests

* remove scripts engine tests

* update errors

* update tests

* refactor tests

* test CreateSnapshot

* add executing block snapshot

* reuse convert functions

* add block_end_snapshot tests

* remove changes

* Apply suggestions from code review

Co-authored-by: Peter Argue <[email protected]>

* add todo

* Update engine/execution/storehouse/block_end_snapshot_test.go

Co-authored-by: Peter Argue <[email protected]>

* Update engine/execution/storehouse/block_end_snapshot_test.go

Co-authored-by: Peter Argue <[email protected]>

* fix lint

* update comment

* add back interface

* update getFromStorage

* handle not found case

* add IsBlockExcuted

* update state commmitment by block id

* update mocks

* fix lint

* update unexecuted_loader

* fix ingestion tests

* fix stop control

* use IsParentExecuted

* chnages to error handling

* Added test

* fix loader tests

* Fix bug in reencoding and update tests

* Fixd remarks

* Added test to emulate errors treated as success for cb

* Added test cases for checking wrong input

* Merged and Linted

* Returned back apiTimeout and used it in connection factory

* Updated test

* Linted

* remove index reporter for registersAsync

* Added parameter blockID to GetSystemTransaction, so it will be more clear that it's return system tx for block, generated mocks, fixed test

* Updated existing functional test to check upstream failover

* use atomic pointer

* Updated documentaion according to comments

* Upgraded godoc

* Replaced check with switch

* Added event with payload to test decoding, linted

* Removed replace and added current version of flow proto

* Replace flow-emulator for integration tests

* changes per suggestions

* correct compareAndSwap

* make tidy

* Fixed errors

* Added more comments for tests

* changed version of flow-emulator

* Removed comments

* modifies the flow-emulator version pinned to match replace statement

See:
- onflow/flow-emulator#514
- #5049

---------

Co-authored-by: ramtinms <[email protected]>
Co-authored-by: Bastian Müller <[email protected]>
Co-authored-by: Alexander Hentschel <[email protected]>
Co-authored-by: Andrii Slisarchuk <[email protected]>
Co-authored-by: Janez Podhostnik <[email protected]>
Co-authored-by: Janez Podhostnik <[email protected]>
Co-authored-by: Ramtin M. Seraj <[email protected]>
Co-authored-by: Yahya Hassanzadeh, Ph.D <[email protected]>
Co-authored-by: Yahya Hassanzadeh <[email protected]>
Co-authored-by: Khalil Claybon <[email protected]>
Co-authored-by: Leo Zhang (zhangchiqing) <[email protected]>
Co-authored-by: Gregor G <[email protected]>
Co-authored-by: Jan Bernatik <[email protected]>
Co-authored-by: Amlandeep Bhadra <[email protected]>
Co-authored-by: Peter Argue <[email protected]>
Co-authored-by: Andrii <[email protected]>
Co-authored-by: UlyanaAndrukhiv <[email protected]>
Co-authored-by: Yurii Oleksyshyn <[email protected]>
Co-authored-by: Amlandeep Bhadra <[email protected]>
Co-authored-by: Andrii Slisarchuk <[email protected]>
Co-authored-by: Kan Zhang <[email protected]>
@peterargue
Copy link
Contributor

Let's wait for that PR to get merged

As far as I can tell, this change is backward-compatible on the flow-emulator side and not backward-compatible on the flow-go side. In these cases, would it not make sense to merge the flow-emulator PR first?

yea, I think that's the way to go

@codecov-commenter
Copy link

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (f1e6dd6) 50.75% compared to head (f5cda71) 50.69%.

Files Patch % Lines
adapters/access.go 0.00% 4 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #514      +/-   ##
==========================================
- Coverage   50.75%   50.69%   -0.06%     
==========================================
  Files          30       30              
  Lines        3933     3937       +4     
==========================================
  Hits         1996     1996              
- Misses       1784     1788       +4     
  Partials      153      153
Flag Coverage Δ
unittests 50.69% <0.00%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@peterargue peterargue mentioned this pull request Nov 28, 2023
Merged
@AndriiDiachuk
Copy link
Collaborator Author

Let's wait for that PR to get merged

@sideninja Could you please merge than, as I need those changes for onflow/flow-go#5049. As @jordanschalm mentioned I can't merge flow-go PR first.

@devbugging
Copy link
Contributor

which PR do you need to be merged? what is the reason for targeting your forked flow-go?

@AndriiDiachuk
Copy link
Collaborator Author

AndriiDiachuk commented Nov 29, 2023 •
edited
Loading

which PR do you need to be merged? what is the reason for targeting your forked flow-go?

I need to merge flow-emulator PR(#514) first, so jobs in PR(onflow/flow-go#5049) won't fail and guys from the team will be able to merge my PR in master without replace statement there. It's not possible to merge into master in flow-go project for current issue without merging the flow-emulator first.

@devbugging
Copy link
Contributor

Looks like the PR you linked is already merged?

@devbugging
Copy link
Contributor

But I agree that shouldn't have a replace directive there, so it's better to have it here temporarily. Let's get this merged targeting flow-go master and then do a follow-up PR on flow-go removing the replace directive.

@AndriiDiachuk
Copy link
Collaborator Author

Looks like the PR you linked is already merged?

It was reverted because of replace statement.

@AndriiDiachuk
Copy link
Collaborator Author

AndriiDiachuk commented Nov 29, 2023 •
edited
Loading

But I agree that shouldn't have a replace directive there, so it's better to have it here temporarily. Let's get this merged targeting flow-go master and then do a follow-up PR on flow-go removing the replace directive.

Yes, that was the idea. Remove it from flow-go after merging flow-emulator. I will remove replace from here right after my PR will be merged in master on flow-go. Thanks for understanding.

@AndriiDiachuk
Copy link
Collaborator Author

Let's wait for that PR to get merged

Could you please merge this PR for now, so I can finish with the corresponding PR in flow-go.

@devbugging
Copy link
Contributor

@AndriiDiachuk once again, what is the corresponding PR on flow-go? before I asked you what is the PR you are waiting for you shared a PR that is now merged into flow-go master, so what PR are you relying on by introducing this replace?

@devbugging
Copy link
Contributor

Looks like the PR you linked is already merged?

It was reverted because of replace statement.

Ahh missed this message. Ok makes sense.

@devbugging devbugging self-requested a review November 29, 2023 12:17
@devbugging devbugging merged commit 196d80d into onflow:master Nov 29, 2023
@devbugging
Copy link
Contributor

@AndriiDiachuk can you please update your flow-go forked version you replace here and make another PR for that since you are now blocking some other features relying on latest flow-go master

@AndriiDiachuk
Copy link
Collaborator Author

AndriiDiachuk commented Nov 29, 2023 •
edited
Loading

@AndriiDiachuk can you please update your flow-go forked version you replace here and make another PR for that since you are now blocking some other features relying on latest flow-go master

Thanks for help with merging. Here is the new PR(onflow/flow-go#5078) for the flow-go without replace statement. Once it is merged, I will create a new PR for the flow-emulator to remove replace statement here as well.

peterargue pushed a commit that referenced this pull request Dec 12, 2023
@devbugging @peterargue
Merge pull request #514 from AndriiDiachuk/AndriiDiachuk/4584-get-blo…
5f80e52 
…ck-endpoint-is-missing-system-collection

[Access] Get Block endpoint is missing the system collection
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devbugging devbugging devbugging approved these changes

@turbolent turbolent Awaiting requested review from turbolent

@janezpodhostnik janezpodhostnik Awaiting requested review from janezpodhostnik janezpodhostnik is a code owner

@bluesign bluesign Awaiting requested review from bluesign bluesign is a code owner

@chasefleming chasefleming Awaiting requested review from chasefleming chasefleming is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@AndriiDiachuk @jordanschalm @peterargue @codecov-commenter @devbugging