feat: make chain-gateways monitornig logic generic for other back-ends - #4266
feat: make chain-gateways monitornig logic generic for other back-ends#4266kevindeforth wants to merge 3 commits into
Conversation
4fc6a64 to
cbe9b9c
Compare
0858244 to
0c67104
Compare
There was a problem hiding this comment.
I left some comments that I thought might be helpful to reviewers. Review recommendation:
- start with the integration tests in
chain-gateway/tests- they should give a good idea of where this is headed on a high-level and show that this doesn't break anything. - The only new stuff this PR adds is the
ViewCallstruct incrates/chain-gateway/src/state_viewer/view_call.rs.
|
|
||
| impl ViewContract for NearViewClientActorHandle { | ||
| type Error = NearViewClientError; | ||
| type ObservedAt = BlockHeight; |
There was a problem hiding this comment.
This was kind of a bug. Implementing ViewContract directly on NearViewClientActor makes it seem like it's okay to call view_contract on the client actor.
But it isn't. It's important we first ensure that the node is fully synced. Otherwise, we risk getting a valid, but historic view, which may lead to all sorts of issues.
In this PR, we preserve the ViewContract implementation over the ChainGateway struct, but delegate this one to a chain-gateway internal method.
| impl Drop for MonitoringTask { | ||
| fn drop(&mut self) { | ||
| self.cancel_token.cancel(); |
There was a problem hiding this comment.
We got rid of this cancel-on-drop implementation by replacing the CancellationToken with its DropGuard.
| /// Holds a Monitoring task and the latest cached value. | ||
| /// This is useful such that we don't unnecessarily deserialize the same state multiple times. | ||
| pub(crate) struct ContractMethodSubscription<Res> { | ||
| inner: MonitoringTask, | ||
| cached: Result<ObservedState<Res>, ChainGatewayError>, | ||
| } | ||
|
|
There was a problem hiding this comment.
This struct was kind of pointless. When I initially introduced this struct, I thought it was needed for testing, but without it, we test just fine, so, simplification wins.
This PR moves the cached field into MontiringTask.
| use near_contract_transport::{ObservedState, TransportError}; | ||
|
|
||
| impl<Res> WatchContractState<Res> for ContractMethodSubscription<Res> | ||
| impl<T, ViewError> WatchContractState<T, ViewError> for MonitoringTask<T, ViewError> |
There was a problem hiding this comment.
Note that this is not the correct place to implement this trait, but I left it here to reduce the diff-size for reviewers. We will move a lot of this to the near-contract-transport crate in a follow-up PR (#4275)
| .changed() | ||
| .await | ||
| .map_err(|_| ChainGatewayError::MonitoringClosed)?; | ||
| .map_err(|_| TransportError::MonitoringClosed)?; |
There was a problem hiding this comment.
And for that, we are now no longer returning a ChainGatewayError, but a TransportError, because the error here is back-end agnostic.
| #[derive(Clone, Debug, Eq, PartialEq)] | ||
| pub struct ObservedState<T = Vec<u8>, H = BlockHeight> { | ||
| pub observed_at: H, | ||
| pub value: T, | ||
| } |
There was a problem hiding this comment.
moved to crates/near-contract-transport/src/views/observation.rs
There was a problem hiding this comment.
This file contains the only innovation in this PR. Everything else is kind of just moving stuff around in a minimally intrusive way to reduce diff size.
Note: I would have preferred to have this file already in the near-contract-transport crate, but that would have required to already move the MonitoringTask, i.e. state_viewer/subscription.rs and state_viewer/monitoring.rs files too, which would have made the diff way bigger.
| let expected_block_time: std::time::Duration = near_config | ||
| .config | ||
| .consensus | ||
| .min_block_production_delay | ||
| .try_into() | ||
| .expect("block time must be non-negative"); |
There was a problem hiding this comment.
Okay, this is also an improvement over the status quo. Instead of hard-coding the poll interval to 200 milliseconds, we read the expected block time and compute a good poll interval from the config value.
There was a problem hiding this comment.
The MockViewContract struct in this file will be moved to the near-contract-transport crate with #4275. That's the reason for breaking it out of the MockChainState.
| pub(crate) async fn view_when_synced<T, F: Future<Output = T>>( | ||
| sync: &impl IsSyncing, | ||
| view: impl FnOnce() -> F, | ||
| ) -> T { | ||
| sync.wait_for_full_sync().await; | ||
| view().await | ||
| } |
There was a problem hiding this comment.
Claude insisted on this, such that we can test the sync-view logic. I agree with claude.
|
@claude review |
This comment was marked as low quality.
This comment was marked as low quality.
|
Finally, Claude didn't find anything of substance, we are good to review 💪 |
Resolves #4253
Main Changes
With #3693 we started unifying calls to our mpc-contract across our various back-ends (e2e-test, sandbox, devnet, node).
As of now, the
near-contract-transportcrate allows one to call a contract and execute one-shot state-views.The node and other components (like the backup-service, which currently uses an RPC endpoint), require not only the ability to view contract state, but also, to monitor contract state for changes.
The chain-gateway offers this capability, but three shortcomings prevent us from moving its traits to the
near-contract-transportcrate, where every back-end could implement them:ChainGatewayError. Other back-ends will have their own error-types. This PR makes the error type a generic that can be set by different back-ends.ViewContract(the backend query),ViewRaw(one-shot raw view),ViewMethod(one-shot call viewing and deserializing state) andSubscribeToContractMethod(monitoring and deserializing state). A user needs to know several different trait-names and what differentiates them:This bears an unnecessary cognitive load. Any struct that provides the
ViewContractinterface should be able to provide state viewing and monitoring for free. In this PR, we introduce the following API:This PR deletes the
SubscribeToContractMethod,ViewMethod, andViewRawtraits, as they are no longer needed. Instead, we introduce aViewCallstruct that can either be.await-ed (to yield the one-shot view response), or.subscribe()-ed to (to yield a continuous view of the contract). TheViewCallallows the caller to define what deserializer to use for deserializing the chain response.Additional changes in this PR:
ViewContractimplementation of theChainGatewaystruct didn't await full-sync before calling the near client actor. This PR fixes that.HasPollIntervaltrait, which allows the consumer to customize how often the monitoring task should query the back-end for changes. This was introduced because the backup-cli relies on a user-provided poll-interval, likely to avoid overwhelming an RPC endpoint with requests.Next steps:
We will want to move a lot of stuff from the
chain-gatewaycrate to thenear-contract-transportcrate. This is purposely left for a follow-up PR, as otherwise, reviewing the changes would become quite cumbersome (c.f. #4275)