Add casper-sse notification trigger for deposits, implement deposit-manager (in favor of #110) #133
Conversation
…tegrate-deploy-notifier-2
marijanp
force-pushed
the
deposit-manager
branch
from
226b0fa to
5e7cbfa
Compare
marijanp
changed the title
There was a problem hiding this comment.
I like most changes, however this PR must be split into few smaller ones. I would expect that adding SSE notification, will just add SSE notification. Here I see that the main processing events logic was refactored (changed and broken), and too many new things were introduced.
| use error::DepositManagerError; | ||
| use kairos_circuit_logic::transactions::{KairosTransaction, L1Deposit}; | ||
|
|
||
| pub struct DepositManager { |
There was a problem hiding this comment.
This should be EventManager, as contract will emit more event types.
There was a problem hiding this comment.
The EventManager as it was/ is implemented now is currently only dealing with Deposit's and it is initialized with a specific contract hash, our current expectation is that the corresponding contract emits deposit events. A EventManager suggests a more generic use case, in that case the EventManager should be capable to handle any kind of events which it isn't doing.
There was a problem hiding this comment.
There was no batch logic in the smart contract yet, so there was no additional event to handle. I designed L1 synchronization mechanism and it should be EventManager.
| &self, | ||
| batch_state_manager: &BatchStateManager, | ||
| ) -> Result<(), DepositManagerError> { | ||
| let last_unprocessed_deposit_index = self.fetcher.fetch_events_count().await?; |
There was a problem hiding this comment.
I would prefer last_unprocessed_event_index and similar naming in the whole function.
There was a problem hiding this comment.
The deposit index is different to the event index. If we decide to emit other events later this deposit counter must not increase. We could add a separate counter and name it last_unprocessed_event_index, but we should not rename this one.
There was a problem hiding this comment.
Unless you always want to match all events before adding Deposits to the batch, then it makes sense.
| } | ||
| } | ||
| } | ||
| self.last_deposit_added_to_batch |
There was a problem hiding this comment.
Internal logic of updating processed events (currently deposits) is broken. Example scenario where there are 3 new events:
- First
fetch_event()succeeds and deposit is added to batch_state_manager - Second
fetch_events()fails (for example transient network issue), so the whole function is finished with error. Counter is not updated! - Next call to
add_events_towill fetch the first event again, and add it to batch_state_manager again.
There was a problem hiding this comment.
Adding events to the batch state manager should be/ is invariant according to @Avi-D-coder. I may understood him wrong. If it's not, I think the better API is to make adding deposits to the batch_state_manager invariant in the batch_state_manager instead of making the hosekeeping of deposits a concern of the user of the API.
There was a problem hiding this comment.
Duplicate deposits added to a batch are not currently handled by the batch state manager. I could de-duplicate them easily.
There was a problem hiding this comment.
Well, instead trying to deduplicate deposits, we should not approve "refactor" code that introduces them.
…last_unprocessed_deposit_index
marijanp
force-pushed
the
deposit-manager
branch
from
74227a3 to
006ab59
Compare
marijanp
force-pushed
the
deposit-manager
branch
from
006ab59 to
548157f
Compare
marijanp
force-pushed
the
deposit-manager
branch
2 times, most recently
from
9d341c2 to
4bde71d
Compare
Things done:
kairos-test-utils/cctlto deploy a given contractEventManagerto be aDepositManager. This manager is part of theServerStateand is used to both fetch events every 30s or when a known deposit deploy event got triggered.cctil-rsand it's implicitly tested in ourkairos-cliintegration test as well as the e2e test