Relayer performs Pyth updates on demand

[peer2f00l]

This change is 

[codecov]

Codecov Report

❌ Patch coverage is 16.93122% with 157 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
service/relayer/src/client/near.rs	0.00%	70 Missing ⚠️
service/relayer/src/app/mod.rs	0.00%	56 Missing ⚠️
service/relayer/src/client/pyth.rs	50.00%	23 Missing ⚠️
service/relayer/src/app/args.rs	0.00%	7 Missing ⚠️
...vice/relayer/src/route/universal_account/create.rs	0.00%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[github-actions]

Name	Deployment
Registry	gh-276.templar-in-training.testnet
Default market	default-19325262469.gh-276.templar-in-training.testnet

---

Installed near-sandbox into /home/runner/work/contracts/contracts/target/debug/build/near-sandbox-utils-2ad3b5a56a37a3e2/out/.near/near-sandbox-2.8.0/near-sandbox

Gas Report

Snapshot Limits

harvest_yield

Iterations	Gas
0	3.4 Tgas
391	14.9 Tgas

Estimated snapshot limit: 9628

apply_interest

Iterations	Gas
0	3.6 Tgas
391	13.2 Tgas

Estimated snapshot limit: 11494

Action Gas Descriptors

Action	Gas
collateralize	12.5 Tgas
withdraw_collateral	9.4 Tgas
borrow	14.9 Tgas
repay	13.8 Tgas
supply	8.6 Tgas
create_supply_withdrawal_request	3.1 Tgas
execute_next_supply_withdrawal_request	11.4 Tgas

Gas Report

Estimated allocation limit: 0

Action Gas Descriptors

Action	Gas
supply	7.9 Tgas
allocate	19.8 Tgas
withdraw	4.0 Tgas
execute withdraw	9.1 Tgas
submit_cap	2.3 Tgas

[carrion256]

service/relayer/src/app/args.rs line 59 at r1 (raw file):

        default_value = "25"
    )]
    pub refresh: Duration,

We should always push on demand. Negating to push it (especially on chains with minimal updates essentially undermines the smoothing & confidence.

Code quote:

    /// Do not push price updates to Pyth oracle if the last push was less
    /// than this long ago, even if requested.
    #[arg(
        id = "pyth-refresh-secs",
        long = "pyth-refresh-secs",
        env = "PYTH_REFRESH_SECS",
        value_parser = duration_from_secs,
        default_value = "25"
    )]
    pub refresh: Duration,

[carrion256]

service/relayer/src/app/mod.rs line 102 at r1 (raw file):

    #[tracing::instrument(skip(self), fields(gas = %gas))]
    pub async fn estimate_cost_of_gas(&self, gas: near_sdk::Gas) -> Option<NearToken> {
        const TERA: u128 = near_sdk::Gas::from_tgas(1).as_gas() as u128;

NIT: not sure what this name even means - perhaps was a constant for the sake of avoiding magic numbers, yet still magic.

[carrion256]

service/relayer/src/app/mod.rs line 269 at r1 (raw file):

                    function_name: call.method_name.clone(),
                });
            }

This forces an iterative style error since it fails fast. I'd suggest to collect all the rejected methods first, then error with an array of indices & names. Otherwise, there could be N calls that can be rejected, yet hidden by this one.

Code quote:

        for (index, call) in calls.into_iter().enumerate() {
            if !contract_data.allowed_methods.contains(&call.method_name) {
                return Err(FunctionCallRejectionReason::UnknownFunctionName {
                    index,
                    function_name: call.method_name.clone(),
                });
            }

[carrion256]

service/relayer/src/app/mod.rs line 330 at r1 (raw file):

                    account_id: receiver_id.clone(),
                });
            }

Ditto here

Perhaps also create a util func

Code quote:

            if !accounts.allowed_contract_data.contains_key(receiver_id) {
                return Err(PayloadRejectionReason::UnknownTransactionReceiverId {
                    account_id: receiver_id.clone(),
                });
            }

[carrion256]

service/relayer/src/app/mod.rs line 362 at r1 (raw file):

                interacted.insert(market_data.collateral_asset.contract_id().to_owned());
            }
            gas += calls.iter().map(|f| f.gas).sum::<u64>();

I think you're accounting for gas even on the rejected calls here.

[carrion256]

service/relayer/src/client/pyth.rs line 24 at r1 (raw file):

            http: reqwest::Client::new(),
            args,
            last_updated: Arc::new(Mutex::new(HashMap::new())),

This is not required if the pyth client is an actor

[carrion256]

service/relayer/src/client/pyth.rs line 86 at r1 (raw file):

        for id in price_ids {
            request = request.query(&[("ids[]", id)]);
        }

This is quite brittle - i'd suggest to use some types from wormhole, they're available as a crate

Code quote:

        for id in price_ids {
            request = request.query(&[("ids[]", id)]);
        }

[carrion256]

@carrion256 reviewed 14 of 14 files at r1, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @peer2f00l)

[peer2f00l]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @carrion256)

---

service/relayer/src/app/args.rs line 59 at r1 (raw file):

Previously, carrion256 (carrion256) wrote…

We should always push on demand. Negating to push it (especially on chains with minimal updates essentially undermines the smoothing & confidence.

I can agree with this to an extent. The intent of this feature is to make sure that if many different users are simultaneously requesting price updates for the same ID that we only update it once (since only one update is necessary to satisfy an unlimited number of simultaneous requests). So, at least, I think it would be fair to reduce the default refresh duration down to something like 3-5 seconds.

---

service/relayer/src/app/mod.rs line 102 at r1 (raw file):

Previously, carrion256 (carrion256) wrote…

NIT: not sure what this name even means - perhaps was a constant for the sake of avoiding magic numbers, yet still magic.

Tera as in Teragas (Tgas)

---

service/relayer/src/app/mod.rs line 269 at r1 (raw file):

Previously, carrion256 (carrion256) wrote…

This forces an iterative style error since it fails fast. I'd suggest to collect all the rejected methods first, then error with an array of indices & names. Otherwise, there could be N calls that can be rejected, yet hidden by this one.

Done.

---

service/relayer/src/app/mod.rs line 362 at r1 (raw file):

Previously, carrion256 (carrion256) wrote…

I think you're accounting for gas even on the rejected calls here.

Shouldn't matter - in the version of the code this comment was made on errors short circuit (so it doesn't matter), or in the new version it will return the list of errors late (still won't matter).

---

service/relayer/src/client/pyth.rs line 24 at r1 (raw file):

Previously, carrion256 (carrion256) wrote…

This is not required if the pyth client is an actor

Assuming by "actor" you mean "sends and receives information exclusively via message-passing" I'm not sure how this follows.

---

service/relayer/src/client/pyth.rs line 86 at r1 (raw file):

Previously, carrion256 (carrion256) wrote…

This is quite brittle - i'd suggest to use some types from wormhole, they're available as a crate

I don't see a crate that does this...

---

service/relayer/src/client/pyth.rs line 86 at r1 (raw file):

Previously, carrion256 (carrion256) wrote…

This is quite brittle - i'd suggest to use some types from pyth, they're available as a crate

I was not able to find an official Pyth crate that supports this. Doesn't seem worth adding a whole dependency for a single deserialization operation.

[carrion256]

service/relayer/src/app/args.rs line 59 at r1 (raw file):

Previously, peer2f00l (peer2) wrote…

I can agree with this to an extent. The intent of this feature is to make sure that if many different users are simultaneously requesting price updates for the same ID that we only update it once (since only one update is necessary to satisfy an unlimited number of simultaneous requests). So, at least, I think it would be fair to reduce the default refresh duration down to something like 3-5 seconds.

Let's reduce it down to three seconds, as we are perhaps prematurely optimising for gas here on a cheap chain.

[carrion256]

service/relayer/src/client/pyth.rs line 24 at r1 (raw file):

Previously, peer2f00l (peer2) wrote…

Assuming by "actor" you mean "sends and receives information exclusively via message-passing" I'm not sure how this follows.

My point that an Arc is an anti pattern which introduces a lot of locking at scale. It usually means that there's poor separation of concerns within asynchronous boundaries and it's better to keep state locally by using different concurrency primitives, if at all. Otherwise, i wouldn't block this pull request but eventually we have tech debt if the relayer requires some real scale (20+ tx/s) as this will eventually lock.

[carrion256]

service/relayer/src/client/pyth.rs line 86 at r1 (raw file):

Previously, peer2f00l (peer2) wrote…

I was not able to find an official Pyth crate that supports this. Doesn't seem worth adding a whole dependency for a single deserialization operation.

Here, i've found it https://github.com/pyth-network/pyth-crosschain/blob/main/apps/hermes/client/rust/src/lib.rs

The problem is not related to complexity of implementation or binary sizes, the problem is maintenance, I'd personally prefer that the core team who run the pyth protocol maintain an integration for me. This allows us to focus within our domain rather than any implementation drift. This means we can just get updates by dependabot and update the relayer when the API changes rather than wait for prices to stop updating, which opens us up to economic attacks in the middle of the night.

[carrion256]

@carrion256 reviewed 1 of 4 files at r2.
Reviewable status: 9 of 14 files reviewed, 5 unresolved discussions (waiting on @peer2f00l)