Refactor to use eventuals for monitoring network data #54
Conversation
Jannis
force-pushed
the
jannis/eventuals
branch
2 times, most recently
from
df129d2 to
6e52bd1
Compare
Specifically, use one eventual for monitoring the indexer's active and recently closed allocations, and one for establishing attestation signers for these allocations.
Jannis
force-pushed
the
jannis/eventuals
branch
from
6e52bd1 to
bd5cd95
Compare
Pull Request Test Coverage Report for Build 6262267806
💛 - Coveralls |
aasseman
added
size:large
| for i in 0..100 { | ||
| // The allocation was either created at the epoch it intended to or one | ||
| // epoch later. So try both both. | ||
| for created_at_epoch in [allocation.created_at_epoch, allocation.created_at_epoch - 1] { |
There was a problem hiding this comment.
I might be missing context for this one even though this is the same as in the ts impl
is created_at_epoch the intended epoch, or created_at_epoch - 1 the intended epoch? trying to wrap my head around the -1 for one epoch later when the epoch increments wrt time
| const INDEXER_OPERATOR_MNEMONIC: &str = "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about"; | ||
|
|
||
| #[test] | ||
| fn test_derive_key_pair() { |
There was a problem hiding this comment.
should we keep these unit tests around?
There was a problem hiding this comment.
I had a bit of a hard time reviewing because there is a lot of changes that are not strictly related to the implementation of eventuals (refactor of the internals of the signers code, network subgraph), and the removal of many tests, some of which I think could have worked without significant modifications, prevents me from checking that the behavior is correct.
In general I prefer single concern PRs as they are more palatable to the reviewer, and it's easier to rollback PRs if there's a problem in the future without rolling back unrelated changes.
| // Keep a cache of the most recent 1000 signers around so we don't need to recreate them | ||
| // every time there is a small change in the allocations | ||
| let cache: &'static Mutex<LruCache<_, _>> = Box::leak(Box::new(Mutex::new(LruCache::new( | ||
| NonZeroUsize::new(1000).unwrap(), | ||
| )))); |
There was a problem hiding this comment.
What is the rationale for using an LRU cache? Wasn't it overall simpler and more efficient to just update a HashMap in place directly? Especially when you consider that the LRU cache is a souped up HashMap itself.
| @@ -61,26 +62,24 @@ async fn main() -> Result<(), std::io::Error> { | |||
| // Make an instance of network subgraph at either | |||
| // graph_node_query_endpoint/subgraphs/id/network_subgraph_deployment | |||
| // or network_subgraph_endpoint | |||
| let network_subgraph = NetworkSubgraph::new( | |||
| let network_subgraph: &'static NetworkSubgraph = Box::leak(Box::new(NetworkSubgraph::new( | |||
There was a problem hiding this comment.
This works, but since NetworkSubgraph is ARC, it could also have been cloned around. Since it indeed is supposed to stick around the whole lifetime of the program it does not technically make a big difference though.
There was a problem hiding this comment.
Perhaps I should have used this pattern since the beginning though, makes a lot of things even simpler. I suppose the only downside is that it makes everything expect a static reference, which can be sometimes limiting in a library?
|
@aasseman I can try to pick the changes apart a bit and submit separate PRs. I like more incremental PRs as well. And yes, I need to invest more into testing again. |
Eventuals are like
tokio::sync::watch, except they only trigger an update downstream if their value changes, not every time a new value is written into it. It's a nice primitive to ensure we always have the latest value available and only do something when there are changes.This switches to eventuals for the following things: