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

Flow API Read Calls Integration with DPS #2567

Closed
AliSMAmin opened this issue Jun 7, 2022 · 7 comments
Closed

Flow API Read Calls Integration with DPS #2567

AliSMAmin opened this issue Jun 7, 2022 · 7 comments

Comments

@AliSMAmin
Copy link

Problem Definition

The legacy Access API contained methods to both read and write (transaction) the blockchain. On Flow, reading is accomplished by read calls or scripts querying the blockchain state. Transactions are write calls which mutate blockchain state.

Without DPS, Access Nodes: forward transactions to Collection Nodes, answer client read calls locally, and delegate script execution to Execution Nodes. This is very read-heavy and poses numerous scalability concerns. When deployed, DPS provides access to the execution state at any block height and does not increase network load.

The Flow API Service, modularized from the legacy Access API, will serve as an entry-point API for transactions, queries, and Cadence scripts. While transactions will still make their way to the network through an Access Node, all other calls should be handled by BDS or DPS if deployed.

Proposed Solution

• Interface on Flow API translates Protocol API and Execution Data API calls into DPS API calls
• Flow API handles routing of scripts (read-only) and transactions (write) to the corresponding target
• Flow API communicates with DPS API for read calls, running against indexed state data
• Leverage DPS for state indexing, reducing load on Access Nodes/Observer Services

Step Definitions

• Flow API can bind to upstream DPS API
• Implement DPS API-Access API translation interface on the Flow API
• If Flow API binds to DPS API, it sends all calls to DPS API except unimplemented methods: GetExecutionResultForBlockID, GetLatestProtocolStateSnapshot, and SendTransaction.
• Endpoints that query protocol state should be translated at the Flow API level and then sent to DPS API for results
• Endpoints that query execution state should be translated at the Flow API level and then sent to DPS API for results

Definition of Done

• Protocol API endpoints which query protocol state are translated by Flow API-DPS API interface and sent to DPS
• Execution Data API endpoints which query execution state are translated by Flow API-DPS API interface and sent to DPS
• Results returned from DPS API are served to clients
• If there is no downstream DPS, an error is returned
• DPS runs state queries from its local IndexDB and returns results through the DPS API.
• DPS indexed blockchain state is used to answer execution state requests from the DPS API.
• Those answers and synced state data are cached for timely future access.

@AliSMAmin
Copy link
Author

All relevant method mappings are delineated in: https://github.com/optakt/flow-dps-access

@AliSMAmin
Copy link
Author

ExecuteScript calls will be dealt with in #2573

@AliSMAmin
Copy link
Author

AliSMAmin commented Jun 8, 2022

Access API-DPS API interface does not implement GetExecutionResultForBlockID and does not contain GetExecutionResultByID.

// GetExecutionResultForBlockID is not implemented.// See https://docs.onflow.org/access-api/#getexecutionresultforblockidfunc (s *Server) GetExecutionResultForBlockID(_ context.Context, req *access.GetExecutionResultForBlockIDRequest) (*access.ExecutionResultForBlockIDResponse, error) { return nil, errors.New("GetExecutionResultForBlockID is not implemented by the Flow DPS API; please use the Flow Access API on a Flow access node directly")}

As per, https://github.com/dapperlabs/flow-dps/blob/master/docs/dps-api.md#endpoints .

GetExecutionResultByID was added on 12/16/21.

GetExecutionResultForBlockID was added on 8/17/21.

Dapper has agreed to contribute to accommodating these APIs on DPS side. This issue tracks the effort to accommodate them on the Flow API side.

@AliSMAmin
Copy link
Author

@A-town21 said,

"Not sure if Miklos is back today, but would like to have some technical discussion. Miklos mentioned using flow-dps to register the grpc server (https://github.com/GetElastech/flow-dps/blob/8bad8bda89e0e7865039db4130d3dbc288db87b2/api/dps/api_grpc.pb.go#L273) versus onflow/Flow AccessAPI which the flow-dps-access appeared to be using initially. Are we still wanting to us flow-dps-access as an interface? If so, I can't seem to get it to register as an access api server, even with the said missing method implemented (see latest failed build for feature/register-grpc-service )- maybe there is some more work could do there to get that running? It seems to implement all of the needed methods. If our new strategy is to abandon flow-dps-access all together and build out the required methods for flow-dps server, we can begin that work- I just want to make sure we have a clear direction here, because these are two different paths. "

@AliSMAmin
Copy link
Author

AliSMAmin commented Jul 13, 2022

This is the failed build?
https://github.com/GetElastech/api-service/actions/runs/2600076711

Aaron: First action here: https://github.com/GetElastech/api-service/actions

Ali:
cannot use dpsServer (variable of type *api.Server) as "github.com/onflow/flow/protobuf/go/flow/access".AccessAPIServer value in argument to access.RegisterAccessAPIServer: *api.Server does not implement "github.com/onflow/flow/protobuf/go/flow/access".AccessAPIServer (missing method GetTransactionResultByIndex) (typecheck)

This seems like an engineering decision as to which one will be more convenient so I don't want to step on
[@miklos Szegedi]'s toes

[Ali Amin] :
Strange that it's still giving an error regarding GetTransactionResultByIndex...

[Aaron LaVallee] :
I know, right? haha

[Aaron LaVallee] :
Sure thing, we can wait for Miklos. It will be good to give Dapper a reason why we are not using flow-dps-access should we decide not to.

[Ali Amin] :
Cursorily it looks like it causes more problems than it solves trying to implement it if we can modify DPS to register the Flow API as a consumer...

[Ali Amin] :
The value in the interface was partially because we didn't have a functional DPS, but now it seems like a middleman that we don't need?

[Aaron LaVallee] :
Yea, that was my thinking as well, and likely why Miklos suggested the same

[Ali Amin] :
I wouldn't recommend we feel bound to implement flow-dps-access since it never really worked in the first place and was more of a prototype. I still would like [@miklos Szegedi] to chime in, but your line of thinking makes sense to me

@szegedim
Copy link
Contributor

szegedim commented Aug 1, 2022

Here is the passing test for this change on the Access APi: https://github.com/GetElastech/api-service/actions/runs/2776003280
@A-town21 is adding the protocol changes. We might have some design suggestions soon. The issue was that we go directly to DPS but it is not set yet in case of the current end-to-end test. This confuses the logic and it returns a not implemented exception.
We should fall back to BDS in such cases.

@miklos
Copy link

miklos commented Oct 11, 2022 via email

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

No branches or pull requests

7 participants
@franklywatson @miklos @j1010001 @szegedim @A-town21 @AliSMAmin and others