adds QGB querier interfaces #45
Conversation
rach-id
added
p2p
| // QuerierI queries the P2P network for confirms. | ||
| // Note: for now, we will be querying the P2P network directly without storing anything in the store. | ||
| // this would make it easier to have a simple working version. Then, we can iterate from there on. | ||
| type QuerierI interface { | ||
| // QueryTwoThirdsDataCommitmentConfirms queries the p2p network for data commitments signed by more | ||
| // than 2/3s of the last validator set. | ||
| // It takes a timeout to wait for validators to sign and broadcast their signatures. | ||
| // Should return an empty slice and no error if it couldn't find enough signatures. | ||
| QueryTwoThirdsDataCommitmentConfirms( | ||
| ctx context.Context, | ||
| timeout time.Duration, | ||
| dc celestiatypes.DataCommitment, | ||
| ) ([]types.DataCommitmentConfirm, error) | ||
|
|
||
| // QueryTwoThirdsValsetConfirms queries the p2p network for valsets signed by more | ||
| // than 2/3s of the last validator set. | ||
| // It takes a timeout to wait for validators to sign and broadcast their signatures. | ||
| // Should return an empty slice and no error if it couldn't find enough signatures. | ||
| QueryTwoThirdsValsetConfirms( | ||
| ctx context.Context, | ||
| timeout time.Duration, | ||
| valset celestiatypes.Valset, | ||
| ) ([]types.ValsetConfirm, error) | ||
|
|
||
| // QueryValsetConfirmByOrchestratorAddress get the valset confirm having nonce `nonce` | ||
| // and signed by the orchestrator whose address is `address`. | ||
| QueryValsetConfirmByOrchestratorAddress( | ||
| ctx context.Context, | ||
| nonce uint64, | ||
| address string, | ||
| ) (*types.ValsetConfirm, error) | ||
|
|
||
| // QueryDataCommitmentConfirmByOrchestratorAddress get the data commitment confirm having nonce `nonce` | ||
| // and signed by the orchestrator whose address is `address`. | ||
| QueryDataCommitmentConfirmByOrchestratorAddress( | ||
| ctx context.Context, | ||
| nonce uint64, | ||
| address string, | ||
| ) (*types.DataCommitmentConfirm, error) | ||
|
|
||
| // QueryDataCommitmentConfirms get all the data commitment confirms in store for a certain nonce. | ||
| QueryDataCommitmentConfirms( | ||
| ctx context.Context, | ||
| nonce uint64, | ||
| ) ([]types.DataCommitmentConfirm, error) | ||
|
|
||
| // QueryValsetConfirms get all the valset confirms in store for a certain nonce. | ||
| QueryValsetConfirms( | ||
| ctx context.Context, | ||
| nonce uint64, | ||
| ) ([]types.ValsetConfirm, error) | ||
| } |
There was a problem hiding this comment.
I'm not sure if this is a good starting point for the interface we're looking for imho. For example, We are not planning on having multiple implementations of QueryForTwoThirds, so I'm not sure atm if it makes sense to have it. It seems like that, and many of the others here, can simply be a function or a method.
If these things are just for testing purposes, then we might still mock a different more standard (smaller) interface
There was a problem hiding this comment.
If these things are just for testing purposes
Yes, 100% they are. As stated here #50 (comment), I would love to see some examples of mocks to improve these big interfaces.
There was a problem hiding this comment.
I think we should just not mock anything, and use the testnode code, which should be pretty easy to setup a testsuite for, and can have super short block times. If we really wanted to get clever, we can store some state/blocks as test data, and then boot up that "frozen" chain.
Alternatively, we could generate mocks for all of the core/app rpc endpoints needed, but that's only marginally better than the other form of mocks and not be worth the effort
| // TmQuerierI queries tendermint for commitments and events. | ||
| type TmQuerierI interface { | ||
| Stop() error | ||
| // QueryCommitment queries tendermint for a commitment for the set of blocks | ||
| // defined by `beginBlock` and `endBlock`. | ||
| QueryCommitment(ctx context.Context, beginBlock uint64, endBlock uint64) (bytes.HexBytes, error) | ||
|
|
||
| // SubscribeEvents subscribe to the events named `eventName`. | ||
| SubscribeEvents(ctx context.Context, subscriptionName string, eventName string) (<-chan coretypes.ResultEvent, error) | ||
| } |
There was a problem hiding this comment.
do you think we should add proofs to this?
There was a problem hiding this comment.
I'm not sure where to put the proof generation code. I was thinking of adding it to V1, but we will be deploying a mocked version of it, should it belong to V1-mocked?
The thing is, the versions have different structures. Thus, it won't be as easy as back porting the PR to the version we want.
What do you think? Where should the command generating the proofs and verifying them be?
There was a problem hiding this comment.
we can add them later, I'm not sure where to add them, but will defer to you
| // QuerierI queries the P2P network for confirms. | ||
| // Note: for now, we will be querying the P2P network directly without storing anything in the store. | ||
| // this would make it easier to have a simple working version. Then, we can iterate from there on. | ||
| type QuerierI interface { |
There was a problem hiding this comment.
[not blocking nit]
The I at the end feels redundant and is usually not added unless there is something else that using the same name. If that's the case, then imho I think we should change one of the names so they don't match
There was a problem hiding this comment.
There will be the implementation of QuerierI called Querier, unless we find a better name for it 🐱 . I agree, the current naming scheme I have been following for these PRs is a bit dull
There was a problem hiding this comment.
okay, yeah its not the end of the world lol
on a unrelated note, why is the querier in the p2p package?
There was a problem hiding this comment.
there are multiple queriers, the tendermint querier (for getting the data commitments, the roots etc from tendermint) and the qgb module querier (for getting the attestations from the state machine) and the p2p querier (for eventually getting the attestations confirms)
|
@evan-forbes For this PR, the feedback is around the interface, which we created mostly for testing but we decided to use a test node. Can we merge this one until we have the test node? |
Overview
Closes #41
Checklist