-
Notifications
You must be signed in to change notification settings - Fork 169
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
Endpoint for EIP-4881 Deposit Contract Snapshot #245
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once merge conflicts are addressed
allOf: | ||
- $ref: "../../beacon-node-oapi.yaml#/components/schemas/ErrorMessage" | ||
- example: | ||
code: 404 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we use 404 for when the "thing" is known not to exist (for example an empty slot) - a different code would probably be useful here for ephemeral error conditions (503/syncing for example)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do tend to agree 404 isn't ideal (a server with a route that is missing is also 404) - even if it was a 400, or potentially 503 might work... I'll see if i can get some more opinions on this one.
@@ -0,0 +1,34 @@ | |||
get: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should there be support for downloading historical snapshots? ie when checkpoint-syncing, one will want to have a matching snapshot to the state that's being synced, which may or may not (due to race conditions or checkpoint age choice) be the same as the snapshot being served by "default"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the main idea is just to get a starting point rather than download the whole deposit log, so I think there's probably not a need currently to get historical snapshots...
resolved conflicts. Let me know if you're interested in continuing discussion or happy to merge @arnetheduck |
There's been no further feedback in more than 10 days. resolved conflicts again and will merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Adds the endpoint for downloading the
EIP-4881
Deposit Tree Snapshot during weak subjectivity sync.