Refactor soroban-simulation to use the e2e_invoke #1354

dmkozh · 2024-02-24T01:53:57Z

What

Refactor soroban-simulation to use the e2e_invoke and also to be more modular/reusable.

Use e2e_invoke for invoking host functions in recording mode
Separate network config and adjustment config into separate structs for easier testing/caching etc.
Clean up the storage logic to clearly distinguish between snapshots with and without archive access
Added some documentation for public functions/structs
Added some basic test coverage. I didn't try to verify the math for every number, but I did sanity-check them. The host function simulation accuracy is mostly covered on e2e_invoke side.
Reuse host code for common test and non-test utilities

Why

Improving simulation quality, as well as maintainability.

Known limitations

N/A

- Simplify `SnapshotSource` interface - Expose some useful functions as public and decoupled from `Host` - Expose most of the test utils for e2e testing

…re modular/reusable. - Use e2e_invoke for invoking host functions in recording mode - Separate network config and adjustment config into separate structs for easier testing/caching etc. - Clean up the storage logic to clearly distinguish between snapshots with and without archive access - Added some documentation for public functions/structs - Added some basic test coverage. I didn't try to verify the math for every number, but I did sanity-check them. The host function simulation accuracy is mostly covered on `e2e_invoke` side.

2opremio · 2024-02-28T14:55:51Z

@dmkozh Nice! I see that the public API has changed. Did you take into consideration the impact at https://github.com/stellar/soroban-rpc/blob/main/cmd/soroban-rpc/lib/preflight/src/lib.rs and whether it satisfies all the current functionality?

2opremio · 2024-02-28T14:57:18Z

@dmkozh I would like to make sure that all integration tests of soroban-rpc pass after these changes. Could you take a stab at adapting the rpc code?

2opremio · 2024-02-28T15:02:48Z

Another option is to do this in a two-step process, keeping an externally-compatible layer at first.

dmkozh · 2024-02-28T17:33:09Z

Did you take into consideration the impact at https://github.com/stellar/soroban-rpc/blob/main/cmd/soroban-rpc/lib/preflight/src/lib.rs and whether it satisfies all the current functionality?

Of course, these changes are meant to be functionally no-op (besides the additional capabilities, like ledger diffs we've discussed before - but there is no need to use them immediately). I'll definitely help with the migration, but this PR has to be finalized first.

This feature has to be used for changes that are considered 'breaking' from the semver standpoint, but those that we still want to integrate to RPC/SDK/Core.

… into sim_refactor

leighmcculloch

A couple asks are inline. At minimum can we fix the docs issue before this is released?

leighmcculloch · 2024-02-28T22:53:14Z

soroban-env-host/Cargo.toml

+# These changes may be used by the 'Soroban-owned' crates, such as
+# stellar-core, soroban-simulation, and soroban-sdk.
+# When bumping the major version of soroban-env-host all the code guarded
+# by this feature should be enabled unconditionally.


It feels odd to say this functionality can only be used by certain crates. Realistically anyone can use this functionality. What we should communicate about is who can safely use this functionality without big surprises or breaking builds.

Suggested change

# These changes may be used by the 'Soroban-owned' crates, such as

# stellar-core, soroban-simulation, and soroban-sdk.

# When bumping the major version of soroban-env-host all the code guarded

# by this feature should be enabled unconditionally.

# These changes should only be used by importers who depend on

# an exact patch version of the crates in this repo, because breaking

# changes may be introduced into any version in any code gated by

# this feature. When bumping the major version of any crates in this

# repo all the code guarded by this feature should be enabled

# unconditionally.

leighmcculloch · 2024-02-28T22:58:27Z

soroban-env-host/src/e2e_invoke.rs

+    #[cfg(any(test, feature = "unstable-next-api"))]
+    pub contract_events_and_return_value_size: u32,


This field will show up in the docs on docs.rs without any disclaimer it is only available with a certain feature. We have a couple options:

Mark any unstable-next-api using the doc attribute so that docs.rs knows that it is only available with certain features and will display so.

This is what we do for testutils in the sdk. It requires more work. It's good for things we want people to use.

See this example for how to do it:
https://github.com/stellar/rs-soroban-sdk/blob/9d802fc8ab82ff571e71b85c790b397b680a00bf/soroban-sdk/src/testutils.rs#L2

Tell docs.rs not to enable the unstable-next-api feature at all when building docs so that none of these features show up in the docs at all.

This is the simpler approach. It's a single line in a toml file. We need to change:

rs-soroban-env/soroban-env-host/Cargo.toml

Lines 122 to 123 in 0ceb08b

[package.metadata.docs.rs]

all-features = true

My gut feel is we should do (2), because we don't see a ton of value in people using this features asap.

leighmcculloch · 2024-02-28T23:00:10Z

soroban-env-host/src/e2e_invoke.rs

+        #[cfg(any(test, feature = "unstable-next-api"))]
+        let entry_with_live_until = init_storage_snapshot.get(key)?;
+        #[cfg(not(any(test, feature = "unstable-next-api")))]
+        let entry_with_live_until = if init_storage_snapshot.has(key)? {


The fact that test is included in here as an any condition suggests that the non-unstable code is not tested. How can we still test it?

I don't think there is anything to test in this particular case. SnapshotSource is not really covered directly. The new field is covered in the tests as well and I don't see much value in covering not returning this field.

### What This is a small followup/cleanup after #1354. Updated comments and exclude some features from the generated docs. ### Why Cleanup ### Known limitations N/A --------- Co-authored-by: Leigh McCulloch <351529+leighmcculloch@users.noreply.github.com>

Refactor soroban-env-host for better simulation support:

e503bf3

- Simplify `SnapshotSource` interface - Expose some useful functions as public and decoupled from `Host` - Expose most of the test utils for e2e testing

dmkozh requested review from graydon, sisuresh and a team as code owners February 24, 2024 01:53

dmkozh force-pushed the sim_refactor branch from 7eb033b to 5c3ab79 Compare February 24, 2024 01:57

dmkozh added 3 commits February 23, 2024 21:04

!fixup Clippy fixes

cd4f0fa

!fixup build fix

e2edf7f

Merge branch 'main' into sim_refactor

66767c1

sisuresh approved these changes Feb 28, 2024

View reviewed changes

dmkozh added 7 commits February 28, 2024 16:35

Fix semver check by introducing an 'unstable' feature.

8b05bee

This feature has to be used for changes that are considered 'breaking' from the semver standpoint, but those that we still want to integrate to RPC/SDK/Core.

Merge branch 'sim_refactor' of https://github.com/dmkozh/rs-soroban-env…

cffecc3

… into sim_refactor

Merge branch 'main' into sim_refactor

2d2e7fa

!fixup build fixes

dbfe6e2

Merge branch 'sim_refactor' of https://github.com/dmkozh/rs-soroban-env…

c968d7e

… into sim_refactor

!fixup clippy fixes

3bdf926

!fixup build fix

790deea

dmkozh added this pull request to the merge queue Feb 28, 2024

Merged via the queue into stellar:main with commit 0ceb08b Feb 28, 2024
13 checks passed

dmkozh deleted the sim_refactor branch February 28, 2024 22:56

leighmcculloch requested changes Feb 28, 2024

View reviewed changes

dmkozh added a commit to dmkozh/rs-soroban-env that referenced this pull request Feb 28, 2024

Cleanup Cargo.toml after stellar#1354

b93d176

dmkozh mentioned this pull request Feb 28, 2024

Cleanup Cargo.toml for unstable-next-api feature #1358

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor soroban-simulation to use the e2e_invoke #1354

Refactor soroban-simulation to use the e2e_invoke #1354

dmkozh commented Feb 24, 2024

2opremio commented Feb 28, 2024 •

edited

Loading

2opremio commented Feb 28, 2024

2opremio commented Feb 28, 2024

dmkozh commented Feb 28, 2024

leighmcculloch left a comment

leighmcculloch Feb 28, 2024 •

edited

Loading

leighmcculloch Feb 28, 2024

leighmcculloch Feb 28, 2024

dmkozh Feb 28, 2024

-# These changes may be used by the 'Soroban-owned' crates, such as
-# stellar-core, soroban-simulation, and soroban-sdk.
-# When bumping the major version of soroban-env-host all the code guarded
-# by this feature should be enabled unconditionally.
+# These changes should only be used by importers who depend on
+# an exact patch version of the crates in this repo, because breaking
+# changes may be introduced into any version in any code gated by
+# this feature. When bumping the major version of any crates in this
+# repo all the code guarded by this feature should be enabled
+# unconditionally.

		#[cfg(any(test, feature = "unstable-next-api"))]
		pub contract_events_and_return_value_size: u32,

Refactor soroban-simulation to use the e2e_invoke #1354

Refactor soroban-simulation to use the e2e_invoke #1354

Conversation

dmkozh commented Feb 24, 2024

What

Why

Known limitations

2opremio commented Feb 28, 2024 • edited Loading

2opremio commented Feb 28, 2024

2opremio commented Feb 28, 2024

dmkozh commented Feb 28, 2024

leighmcculloch left a comment

Choose a reason for hiding this comment

leighmcculloch Feb 28, 2024 • edited Loading

Choose a reason for hiding this comment

leighmcculloch Feb 28, 2024

Choose a reason for hiding this comment

leighmcculloch Feb 28, 2024

Choose a reason for hiding this comment

dmkozh Feb 28, 2024

Choose a reason for hiding this comment

2opremio commented Feb 28, 2024 •

edited

Loading

leighmcculloch Feb 28, 2024 •

edited

Loading