feat(Gas Oracle): Remove Fetcher singleton #1360
Conversation
…emove-token-fetcher-singleton
| current_try: AtomicU8, | ||
| alert_spawned: AtomicBool, |
There was a problem hiding this comment.
Nit: I think having a single atomic variable to implement interior mutability is usually fine, but two is sus (especially given that if this would ever actually be used in a shared context, it may introduce bugs). Probably the right approach here would be to put it under Mutex (actually, RefCell should be fine since we won't hold the guard across an await point, but up to you).
There was a problem hiding this comment.
After looking into it a bit, I realized the way the logic worked, the alert_spawned boolean could be removed, as essentially all we are doing is sending an alert when the MAX_CONSECUTIVE_NETWORK_ERRORS threshold is reached.
Removed it here 6e0c812 so now there's just one atomic variable.
There was a problem hiding this comment.
I'm not sure if the new code is equivalent to the old behavior. Given that the variable type is u8, with polling period of 3 seconds it'll overflow in ~13 minutes and spawn an alert again. The new logic also uses non-atomic increments through plain u8, which means that in debug mode it'll panic (as Rust in debug mode checks for integer overflows), which is probably not desired.
I think it's better to still keep two variables and use a lock on the structure. Given that there will be only a single consumer for this lock, it's not a subject for contention and overall it's not a performance-critical part of the code.
There was a problem hiding this comment.
Got it, I reverted the changes and went for an Arc<Mutex> implementation here fbf66b1
etc/env/l1-inits/.init.env
Outdated
| @@ -1,29 +1,29 @@ | |||
| CONTRACTS_LATEST_PROTOCOL_VERSION=22 | |||
| CONTRACTS_CREATE2_FACTORY_ADDR=0x3c4CE2E9D8b03BbCd568E47465A483EE86893c49 | |||
| CONTRACTS_VERIFIER_ADDR=0xCf42AEFfB17dED8de2d4116D33716354F53e51e6 | |||
| CONTRACTS_L1_MULTICALL3_ADDR=0x69D85c64032337E58064A35020d6F45Dab350883 | |||
| CONTRACTS_VERIFIER_ADDR=0xF025a57cfda291db967Cbd88120fb8997232bC5E | |||
There was a problem hiding this comment.
Not sure if this file was intended to be committed to the repo in the first place, looks like it should've been added to .gitignore.
There was a problem hiding this comment.
I reverted the changes to the file here a19be60, I believe it was commited in the shared bridge PR for some reason.
| sync::{watch, OnceCell}, | ||
| task::JoinHandle, | ||
| }; | ||
| use tokio::sync::Mutex; |
There was a problem hiding this comment.
Side comment: mutexes from std are perfectly fine to use in an async context, and generally they are more performant than tokio mutex. Usually, you only need tokio mutex if you need to hold a guard across an await point (and even in such situations, it's often a code smell). See the tokio docs for context.
This is a minor thing though and can be kept as is.
| version "8.56.0" | ||
| resolved "https://registry.yarnpkg.com/@eslint/js/-/js-8.56.0.tgz#ef20350fec605a7f7035a01764731b2de0f3782b" | ||
| integrity sha512-gMsVel9D7f2HLkBma9VbtzZRehRogVRfbr++f06nL2vnCGCNlzOD+/MUov/F4p8myyAHspEhVobgjpX64q5m6A== | ||
| "@eslint/js@8.57.0": |
There was a problem hiding this comment.
Not sure why this file has changed. Nothing wrong with bumping dependencies, just looks a bit unrelated.
Can be kept as is.
…emove-token-fetcher-singleton
jrchatruc
merged commit ba7762c
into
matter-labs:lambdaclass_gas_oracle
What ❔
This PR removes the
NativeTokenFetcherSingletonas per the discussion here