Remove explicit beacon endpoint and enhance eth/head endpoint instead. #27
Conversation
luka-ethernal
commented
Aug 26, 2024
•
luka-ethernal
commented
- Create a task that periodically checks for changes in head and updates one global state value.
- Make eth/head endpoint just read the current state instead of calling Node every time.
- Integrate a beacon mapping into eth/head endpoint, so mapping becomes easier and atomic.
- Implement a retry mechanism in case of errors.
- If all retries fail, crash the entire process.
- Introduce versioning to endpoints.
…nt from now on. Removed Beaconcha endpoint. Fix documentation. Add explicit retry settings.
src/main.rs
Outdated
| .route("/versions", get(versions)) | ||
| .route("/v1/info", get(info)) | ||
| .route("/v1/eth/proof/:block_hash", get(get_eth_proof)) | ||
| .route("/v1/eth/head", get(get_eth_head)) | ||
| .route("/v1/avl/head", get(get_avl_head)) | ||
| .route("/v1/avl/proof/:block_hash/:message_id", get(get_avl_proof)) |
There was a problem hiding this comment.
Changing URLs will have an impact on our monitors as well.
We should run the old and new bridge API in parallel until the migration is completed.
There was a problem hiding this comment.
Yes, this is on purpose, to let us know what else will need changing. Also, in the future, we need versioning for introducing new functionality and changing old one.
There was a problem hiding this comment.
@luka-ethernal yes but I think @0xSasaPrsic is saying something else, it's that we also need non-v1 endpoints for a deprecation period before we fully switch over. I agree as well, we need both for now.
There was a problem hiding this comment.
Yes, I see. I was thinking we add versioning now and skip the deprecation phase since we have no external users yet (do we?). But if needed, I can do it.
There was a problem hiding this comment.
Tagging @vthunder to help with a suggestion.
There was a problem hiding this comment.
We use it in almost all our validium integrations, all of which are internal (some externally but private) but until we change all of them (in a month maybe?), good to retain.
There was a problem hiding this comment.
Ok, then I'll maintain the old endpoints till we can phase them out. I will need to change the docs as well and bring back the beacon endpoint.
src/main.rs
Outdated
| .route("/versions", get(versions)) | ||
| .route("/v1/info", get(info)) | ||
| .route("/v1/eth/proof/:block_hash", get(get_eth_proof)) | ||
| .route("/v1/eth/head", get(get_eth_head)) | ||
| .route("/v1/avl/head", get(get_avl_head)) | ||
| .route("/v1/avl/proof/:block_hash/:message_id", get(get_avl_proof)) |
There was a problem hiding this comment.
@luka-ethernal yes but I think @0xSasaPrsic is saying something else, it's that we also need non-v1 endpoints for a deprecation period before we fully switch over. I agree as well, we need both for now.
src/main.rs
Outdated
| tracing::info!("🚀 Listening on {} port {}", host, port); | ||
| axum::serve(listener, app).await.unwrap(); | ||
| } | ||
|
|
||
| lazy_static! { | ||
| static ref SLOT_BLOCK_HEAD: Mutex<Option<(u64, u64, B256, u64)>> = Mutex::new(None); |
There was a problem hiding this comment.
Should we use Arc<RwLock> here instead?
There was a problem hiding this comment.
I was thinking about that, right now, it doesn't make a huge difference, but it might in the future. Will change that.
There was a problem hiding this comment.
Since we already use the parking_lot feature in tokio might be better to use parking_lot::RwLock
There was a problem hiding this comment.
Shouldn't the tokio::sync::RwLock use parking_lot on the low level? I can also introduce parking_lot separately, but I see no need.
src/main.rs
Outdated
| @@ -675,30 +538,139 @@ async fn main() { | |||
| .unwrap_or(172800), | |||
| beaconchain_api_key: env::var("BEACONCHAIN_API_KEY").unwrap_or("".to_owned()), | |||
| }); | |||
| tracing::info!("Config: {shared_state:?}"); | |||
There was a problem hiding this comment.
Should not log credentials to logs!
There was a problem hiding this comment.
Good catch. I should probably also remove the Beaconcha api key from code, since it won't be needed.
There was a problem hiding this comment.
(it's also the only secret in this shared state)
|
@QEDK I've addressed all of the comments, please review them if you can. |
…th/head endpoint now uses new mechanism, but has old behavior.
luka-ethernal
force-pushed
the
luka/refactor-beacon-and-head
branch
from
56f5cdb to
d91a81f
Compare
