Conversation
ASuciuX
commented
Aug 25, 2025
ASuciuX
commented
- add the functionality for Implement Redis Notifier for New Block Indexing (for Chainhooks) #551
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
Vercel deployment URL: https://bitcoin-indexer-4t9up77dy-hirosystems.vercel.app 🚀 |
rafaelcr
left a comment
rafaelcr
left a comment
There was a problem hiding this comment.
Looking good! Left a few comments
| let client = Client::open(redis.url.as_str()) | ||
| .map_err(|e| format!("unable to connect to redis: {e}"))?; |
There was a problem hiding this comment.
How does this client handle reconnections? Also, we need to add support for High Availability Redis (Sentinels or Cluster) in these connection options
| bitcoin::Network::Signet => "signet", | ||
| bitcoin::Network::Regtest => "regtest", |
There was a problem hiding this comment.
These will not be supported for now, can you add a todo!() here instead?
| apply_blocks: apply_blocks | ||
| .iter() | ||
| .map(|b| BlockIndexRef { | ||
| hash: b.hash.clone(), |
There was a problem hiding this comment.
we need to make sure these block hashes start with 0x, can you double check if this is the case?
| pub enabled: bool, | ||
| pub url: String, | ||
| pub queue: String, |
There was a problem hiding this comment.
Same here about HA Redis
components/ordinals/src/lib.rs
Outdated
| } | ||
| // Notify redis with both apply and rollback blocks, if enabled | ||
| if let Some(redis_cfg) = &config_moved.redis { | ||
| let notifier = RedisNotifier::new(redis_cfg)?; |
There was a problem hiding this comment.
You should create the notifier only once and reuse it across all block indexing
components/runes/src/lib.rs
Outdated
| } | ||
| // Notify redis with both apply and rollback blocks, if enabled | ||
| if let Some(redis_cfg) = &config_moved.redis { | ||
| let notifier = RedisNotifier::new(redis_cfg)?; |
+ network regtest for unit tests
3 participants
