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

feat(api): first action handlers #7

Merged
merged 10 commits into from
Jan 9, 2025
Merged

feat(api): first action handlers #7

merged 10 commits into from
Jan 9, 2025

Conversation

merklefruit
Copy link
Contributor

@merklefruit merklefruit commented Jan 8, 2025

@merklefruit merklefruit changed the base branch from main to feat/db-conn January 8, 2025 10:39
@mempirate
Copy link
Contributor

Quick note: for any actions on the registry state (reading / writing), the idea is to first call handle.wait_for_sync().await, to ensure that the DB has been synced at the start of the epoch before doing any DB operations. I feel like this will eliminate some race conditions

@merklefruit merklefruit force-pushed the feat/db-conn branch 3 times, most recently from 77b25e6 to e90f4f3 Compare January 8, 2025 13:36
@merklefruit merklefruit force-pushed the feat/action-handlers branch from ba22856 to 957022d Compare January 8, 2025 13:42
Base automatically changed from feat/db-conn to main January 8, 2025 14:05
@merklefruit
Copy link
Contributor Author

Quick note: for any actions on the registry state (reading / writing), the idea is to first call handle.wait_for_sync().await, to ensure that the DB has been synced at the start of the epoch before doing any DB operations. I feel like this will eliminate some race conditions

I see, it makes sense! However as it currently stands this comes with the drawback that if we are still syncing the API will just hang until it is synced. The only good alternative I see is to perhaps return an error saying that the registry is still syncing and to try again later. Wdyt?

@mempirate
Copy link
Contributor

Yeah that's the drawback indeed. I think waiting for a second (hopefully not more than that) might be a better option no? Or we return HTTP Too Early, which would require the client to retry.

@merklefruit
Copy link
Contributor Author

I like the timeout option, and didn't know about that specific HTTP code. nice!

@merklefruit merklefruit marked this pull request as ready for review January 8, 2025 15:48
@merklefruit merklefruit requested a review from mempirate January 8, 2025 15:48
src/main.rs Outdated
Comment on lines 63 to 65
// TODO: fetch pubkeys from indices from beacon node
// let res = registry.get_validators_by_indices(indices).await;
// let _ = response.send(res);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also do this at registration time (& at sync time for external sources) and save them in the DB. We might actually have to do that anyway to check if the validator is valid.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#11

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed in #12

src/registry.rs Outdated

pub(crate) async fn get_registrations(
&mut self,
pubkeys: Option<&[BlsPublicKey]>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this an Option?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, if None we return everything. I think we should disambiguate with list_registrations, list_validators, list_operators instead (which take no arguments)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in latest commit

@merklefruit merklefruit requested a review from mempirate January 9, 2025 10:31
@mempirate mempirate merged commit ca3c305 into main Jan 9, 2025
0 of 3 checks passed
@mempirate mempirate deleted the feat/action-handlers branch January 9, 2025 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Action handling
2 participants