Expose invocation metering in the SDK. #1411

dmkozh · 2024-12-12T00:33:38Z

What

Expose invocation metering in the SDK.

Why

Improving testing UX.

#1319

Known limitations

N/A

leighmcculloch

💯 This is so needed and awesome to see getting added to the SDK.

I have some thoughts just about the API and how we integrate it. Lmk what you think, and we can DM / chat too to do a little more realtime.

leighmcculloch · 2024-12-12T02:20:35Z

soroban-sdk/src/env.rs

+    /// keep in mind that resource and fee estimation may be imprecise. Use
+    /// simulation with RPC in order to get the exact resources for submitting
+    /// the transactions to the network.
+    pub fn enable_invocation_metering(&self) {


What cost does having this enabled incur? Is it noticeable? If it's not material we should enable this by default.

Instead of a function here, I think we should move this into the EnvTestConfig. It'll keep the surface area of testutils functions lower, and is then easily discoverable there. But I'd still have it enabled by default.

The cost is not material, but this does change the observed behavior as the budget will be reset in-between invocations. So in theory this might break someone's tests, which is why I was hesitant about enabling this by default (maybe that should have been enabled in the major release, but now we have to wait until the next one).

the budget will be reset in-between invocations

I think this is a good thing, we should I think have some consistency across these types of functions where top-level invocation is seen as a single unit, much like how you can get auth for the last invocation.

maybe that should have been enabled in the major release

+1.

Yeah, I think we're on the same page here, I'd like to enable this by default ASAP, but probably it's safer to wait until the next release. We can make an announcement though and add some examples.

leighmcculloch · 2024-12-12T02:33:58Z

soroban-sdk/src/env.rs

+    /// contract is used instead of a Wasm contract, all the costs related to
+    /// VM instantiation and execution, as well as Wasm reads/rent bumps will be
+    /// missed.    
+    pub fn get_last_invocation_resources(&self) -> Option<InvocationResources> {


I'd like to move the functions for getting the resources used into some other type which is available on Env, because otherwise Env becomes a bit of a function soup over time.

We currently have env.budget() that returns a testutils Budget (defined in this repo, not the same as the env's Budget). Should these functions replace that? Or can we reimagine both together in a single location? Budget getting the raw usage, and this doing something else? From best I can tell looks like this replaces the existing Budget object.

From best I can tell looks like this replaces the existing Budget object.

Not quite, Budget provides the detailed breakdown of the CPU costs, while this provides a high level overview of all the resources used. I think both have a place in development process.

That said, I agree that we could try converging this functionality at the SDK level, especially given that with this change everything will be describing the last invocation only.

Wdyt of:

env.cost().resources() // returns the new resources structure env.cost().fee() // returns estimated fees env.cost().metering() // returns the existing raw budget

That seems fine, though I'd like to emphasize that these are estimates so it's hopefully not as tempting to use this instead of simulation. So I ended up with cost_estimate().

I've rebased everything from scratch as I was debugging some silly issues, PTAL.

So I ended up with cost_estimate().

👍🏻 I think that's wise to call out their estimates.

soroban-sdk/src/cost_estimate.rs

jayz22 · 2024-12-13T20:59:11Z

soroban-sdk/src/cost_estimate.rs

+    /// VM instantiation and execution, as well as Wasm reads/rent bumps will be
+    /// missed.    
+    pub fn fee(&self) -> Option<FeeEstimate> {
+        // This is a snapshot of the fees as of 2024-12-11.


Instead of hard coding these values, should we extract them into snapshots (with different timestamps), and allow the user to specify one of these or override it with their own values?

I don't think that's necessary. We still need some defaults as setting up the real ledger snapshots for simple unit tests is cumbersome. We could provide a way of overriding this, but I don't see much value in that yet; fees don't change too often and we're in a much better position to update the values in the SDK just once for everyone, than deferring that to the users. We can add the override functionality later if necessary.

+1 that might be necessary in the future, but isn't necessary today.

Same comment as above about the Option<> return value, I think we should unwrap and panic.

jayz22 · 2024-12-13T21:09:24Z

soroban-sdk/src/cost_estimate.rs

+    /// value. If there was no contract call, then the resulting value will
+    /// correspond to metering any environment setup that has been made thus
+    /// far.
+    pub fn detailed_metering(&self) -> Budget {


Is this necessary since there is already an Env::budget() function

Env::budget() can be deprecated in favor of more consistent interface.

Got it. The budget contains metering info as well as a bunch of auxiliary things (such as the cost schedule and trackers) that the user won't need (and might be confused to equate it with "detailed_metering"), and you can call methods on it.
For the sake of UX, instead of returning the full Budget, it may be better to create a read-only "detailed_metering" type that just contains cpu and mem consumption for various costs, similar what's being shown by Budget::Display.

Maybe it could be better if we designed this from scratch, but currently this would just duplicate the functionality that we already have in budget. Maybe we just should leave budget as is and not try to shove it into the new class, as I agree that it seems kind of odd to have the same exact thing in two different places (and we can't easily get rid of budget either as it would be a breaking change). WDYT @leighmcculloch ?

+1 we can deprecate Env::budget() in this PR and say to use the new interface. It's helpful to group these together into a single location to look at this kind of stuff.

Co-authored-by: Jay Geng <[email protected]>

leighmcculloch

👏🏻 Love the level of docs we're adding around these functions.

leighmcculloch · 2024-12-14T03:11:21Z

soroban-sdk/src/cost_estimate.rs

@@ -0,0 +1,111 @@
+use soroban_env_host::{fees::FeeConfiguration, FeeEstimate, InvocationResources};


Can we move this file into the testutils directory? We keep as much as possible in there for testutils only stuff, and everything in that directory will be marked as testutils only in docs so folks don't think it's usable outside tests.

leighmcculloch · 2024-12-14T03:11:47Z

soroban-sdk/src/cost_estimate.rs

+    /// keep in mind that resource and fee estimation may be imprecise. Use
+    /// simulation with RPC in order to get the exact resources for submitting
+    /// the transactions to the network.    
+    pub fn enable(&self) {


We should make this function pub(crate) since it's intended use is via the EnvTestConfig.

leighmcculloch · 2024-12-14T03:13:14Z

soroban-sdk/src/cost_estimate.rs

+    /// contract is used instead of a Wasm contract, all the costs related to
+    /// VM instantiation and execution, as well as Wasm reads/rent bumps will be
+    /// missed.    
+    pub fn resources(&self) -> Option<InvocationResources> {


I think we should return the value, and panic on incorrect use.

Why: Convenience, and this is in tests, so the convenience of not having to unwrap something all the time is a big benefit. If someone makes a mistake it'll be in a test and they'll be able to fix it straight away. The panic can tell them what needs enabling exactly.

leighmcculloch · 2024-12-14T03:14:09Z

soroban-sdk/src/cost_estimate.rs

+    /// VM instantiation and execution, as well as Wasm reads/rent bumps will be
+    /// missed.    
+    pub fn fee(&self) -> Option<FeeEstimate> {
+        // This is a snapshot of the fees as of 2024-12-11.


+1 that might be necessary in the future, but isn't necessary today.

Same comment as above about the Option<> return value, I think we should unwrap and panic.

leighmcculloch · 2024-12-14T03:15:54Z

soroban-sdk/src/lib.rs

+#[cfg(any(test, feature = "testutils"))]
+pub mod cost_estimate;


Move this into the testutils directory and you wo'nt need a cfg testutils here.

Also note that whenever we cfg testutils, we also need to cfg_attr a doc attribute to tell the docs generation that it's only usable in testutils. You won't need to do that if you move it into the testutils directory, but just a knowledge share.

leighmcculloch · 2024-12-14T03:17:35Z

soroban-sdk/test_wasms/README.md

+Files contained in this directory are used in a few SDK tests that are sensitive
+to Wasm content changes.
+
+`test_contract_data.wasm` is a build of `contract_data`  test contract.


We don't need to prebuild the wasm and commit it. All of CI runs make build before the tests, and we have other tests that read wasms straight out of the target directory, so should follow suit.

leighmcculloch · 2024-12-14T03:18:58Z

soroban-sdk/src/cost_estimate.rs

+    /// value. If there was no contract call, then the resulting value will
+    /// correspond to metering any environment setup that has been made thus
+    /// far.
+    pub fn detailed_metering(&self) -> Budget {


+1 we can deprecate Env::budget() in this PR and say to use the new interface. It's helpful to group these together into a single location to look at this kind of stuff.

leighmcculloch · 2024-12-14T03:20:38Z

soroban-sdk/Cargo.toml

@@ -53,6 +53,7 @@ derive_arbitrary = { version = "~1.3.0" }
 proptest = "1.2.0"
 proptest-arbitrary-interop = "0.1.0"
 libfuzzer-sys = "0.4.7"
+expect-test = "1.4.1"


👏🏻 This is pretty cool, love it's use here, seems very appropriate for these types of test expectations.

leighmcculloch reviewed Dec 12, 2024

View reviewed changes

dmkozh force-pushed the resource_metering branch 4 times, most recently from c249cb6 to 4939279 Compare December 13, 2024 18:38

Expose invocation metering in the SDK.

b454ab5

dmkozh force-pushed the resource_metering branch from 4939279 to b454ab5 Compare December 13, 2024 18:42

jayz22 reviewed Dec 13, 2024

View reviewed changes

Update soroban-sdk/src/cost_estimate.rs

9ac0b50

Co-authored-by: Jay Geng <[email protected]>

leighmcculloch reviewed Dec 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose invocation metering in the SDK. #1411

Expose invocation metering in the SDK. #1411

dmkozh commented Dec 12, 2024

leighmcculloch left a comment

leighmcculloch Dec 12, 2024

dmkozh Dec 12, 2024

leighmcculloch Dec 12, 2024

dmkozh Dec 13, 2024

leighmcculloch Dec 12, 2024

dmkozh Dec 12, 2024

leighmcculloch Dec 13, 2024

dmkozh Dec 13, 2024

leighmcculloch Dec 14, 2024

jayz22 Dec 13, 2024

dmkozh Dec 13, 2024 •

edited

Loading

leighmcculloch Dec 14, 2024

jayz22 Dec 13, 2024

dmkozh Dec 13, 2024

jayz22 Dec 13, 2024

dmkozh Dec 13, 2024

leighmcculloch Dec 14, 2024

leighmcculloch left a comment •

edited

Loading

leighmcculloch Dec 14, 2024

leighmcculloch Dec 14, 2024

leighmcculloch Dec 14, 2024

leighmcculloch Dec 14, 2024

leighmcculloch Dec 14, 2024

leighmcculloch Dec 14, 2024

leighmcculloch Dec 14, 2024

leighmcculloch Dec 14, 2024

		@@ -0,0 +1,111 @@
		use soroban_env_host::{fees::FeeConfiguration, FeeEstimate, InvocationResources};

		#[cfg(any(test, feature = "testutils"))]
		pub mod cost_estimate;

Expose invocation metering in the SDK. #1411

Are you sure you want to change the base?

Expose invocation metering in the SDK. #1411

Conversation

dmkozh commented Dec 12, 2024

What

Why

Known limitations

leighmcculloch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmkozh Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leighmcculloch left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmkozh Dec 13, 2024 •

edited

Loading

leighmcculloch left a comment •

edited

Loading