leighmcculloch
commented
4 months ago I have tested this change with the soroban-examples repo, and all the contracts build and pass their tests.
However, when I check that the binary result is identical, there are four contracts that are different. I'll investigate why, because this change should have resulted in near identical code after expansion.
864ec5cb70c03e01e5b95f65f7c8e332b2cadbb591ea5106a033435557b435fd account/target/wasm32-unknown-unknown/release/soroban_account_contract.wasm
-2fcb210308121cea203f2d15bbd47cd7d8826e371b6fb8b2853a48ed43ca96a3 alloc/target/wasm32-unknown-unknown/release/soroban_alloc_contract.wasm
+feb7ec94e625aa10936f3b3c798636f99804ae5386c9d75d1ddb86f982bc97ae alloc/target/wasm32-unknown-unknown/release/soroban_alloc_contract.wasm
902121bc460579baddc5264b891f4600354d504709f0acd91f76857b9028652c atomic_multiswap/target/wasm32-unknown-unknown/release/soroban_atomic_multiswap_contract.wasm
09bbacaf8210ca09a89fcaf75b1a0ed3a58c16ebb6acd274975f638b31ef7eff atomic_swap/target/wasm32-unknown-unknown/release/soroban_atomic_swap_contract.wasm
9ca076e220a60e4a21d79bee06ca7440fa1e12bc31e61b589d6d89651b1cb7c9 auth/target/wasm32-unknown-unknown/release/soroban_auth_contract.wasm
30f9117bd73e147408410b13f83a2a2ea2a55b800c760194b6227e612b6d687d cross_contract/contract_a/target/wasm32-unknown-unknown/release/soroban_cross_contract_a_contract.wasm
3f54f1e0873b35fbb67bb4dda4a2b65a5d62b6c861bc48b8d4fd4bf2a6016373 cross_contract/contract_b/target/wasm32-unknown-unknown/release/soroban_cross_contract_b_contract.wasm
-9d87aeb7e7421dec58ffbb4c263cfb07d00842ba4efc065e7dded820c4d38cb9 custom_types/target/wasm32-unknown-unknown/release/soroban_custom_types_contract.wasm
+5811ec468f1be123bae28764d6ea323281f875f0aca81c8cfbe8c19f0dd45ee5 custom_types/target/wasm32-unknown-unknown/release/soroban_custom_types_contract.wasm
8794701c9b20e25526206ddcefc474d9f956414064cfd39c6bb935b7e25531a9 deep_contract_auth/target/wasm32-unknown-unknown/release/soroban_deployer_contract.wasm
442e3ea5d299633e8343308a47aef5fe475a3a0d55d79da793688f9e23c11b47 deployer/contract/target/wasm32-unknown-unknown/release/soroban_deployer_test_contract.wasm
c9c636fcc81a015e01a330db0219b632a8cb56d3ffd46bee3549832ad7db70a4 deployer/deployer/target/wasm32-unknown-unknown/release/soroban_deployer_contract.wasm
@@ -15,12 +15,12 @@ a3b977d04067846208c0d99a68cdedcb97753ff1c1b017e98bf3ae8a66b0fb69 events/target/
7048ebd3175259506075ad802b9a03c2aa51110a649ea97c876b4159ee22b4f9 fuzzing/target/wasm32-unknown-unknown/release/soroban_fuzzing_contract.wasm
da3c598571f5367da3fd05b4c2bb3278c368cf89918115c08297187f4076e3c0 hello_world/target/wasm32-unknown-unknown/release/soroban_hello_world_contract.wasm
6ad61a638324bdb1af2263a4da7a1d95b1cec53fbd6182094f0a56fb3632576c increment/target/wasm32-unknown-unknown/release/soroban_increment_contract.wasm
-01ea5dd6e5f400131ee21860a31d3aa802d9561c954dc2d2c57b24ea82c5c882 liquidity_pool/target/wasm32-unknown-unknown/release/soroban_liquidity_pool_contract.wasm
+4caa0abd6cd1a721e02f51201b57e7c68c89bd852edf48ad71fa103656708cb2 liquidity_pool/target/wasm32-unknown-unknown/release/soroban_liquidity_pool_contract.wasm
510caa2a6cb5ecd11faa51a946c3f3e711b84ac5026162b7556e598b88991907 logging/target/wasm32-unknown-unknown/release/soroban_logging_contract.wasm
6df170a3117d6b9f3fe437bfc4b6c417073651acfbfee33723e085558cec2cd4 mint-lock/target/wasm32-unknown-unknown/release/soroban_custom_types_contract.wasm
dbcd96bd49421eec6b974942c3ef2566e54f266f25f22c2913468fb99c6cbc76 mint-lock/target/wasm32-unknown-unknown/release/soroban_mint_lock_contract.wasm
477a48286f4d58b557068e2132c2c5690bb67b6d3daf3056dea1ba2726338c91 simple_account/target/wasm32-unknown-unknown/release/soroban_simple_account_contract.wasm
-9384a05a864e5c6ad5932038b9754c429947e440477ad7ce0a4842651912efd1 single_offer/target/wasm32-unknown-unknown/release/soroban_single_offer_contract.wasm
+2a3c20a683cea40f878c75fe2391bfce6e7c43c7400451bf1b5c9f1993b6cc04 single_offer/target/wasm32-unknown-unknown/release/soroban_single_offer_contract.wasm
a6f2d3b402a26757fe56afce1111fdb635254b0b43950a64286e58ddde023931 timelock/target/wasm32-unknown-unknown/release/soroban_timelock_contract.wasm
5f7fc8ca6d789874e99d3f713189e8f15e171a79aa01d5d0b813f9f7ea20585e token/target/wasm32-unknown-unknown/release/soroban_token_contract.wasm
5e1cf5765fb1f3c4d95655bec8cdfeff9703eb13196acfb92f9cedc0825e084a ttl/target/wasm32-unknown-unknown/release/soroban_ttl_example.wasm
What
Change how testutils code is generated to make it rely only on the state of the SDK testutils feature, and not influenced at all by the contract crate's feature set or test cfg.
Today the SDK macros generate both non-testutils and testutils code and it is up to the contract crate to enable or disable it. With this change the SDK macros only generate testutils code if the SDK is compiled with testutils.
Why
Today both the SDK and the contract crate must be configured with the testutils feature (or the test cfg) to enable the testutils that are generated by the SDK macros.
The SDK has a testutils feature that enables the testutils functions and types in the SDK.
The contract crate generated code also has code behind a testutils feature, and the contract crate testutils feature (or test cfg) must be enabled for them to function as well.
This largely works, but it results in an oddity in more complex workspace setups that you can be using a testutils SDK with a non-testutils compiled contract crate meaning that none of the types or functions in that crate can be used with testutils functions.
This largely goes unnoticed because it is a bit of an edge case and definitely not apparent in simpler setups. Even larger setups may never notice this as an oddity if they always happen to configure their crates appropriately.
This also goes unnoticed because when the test cfg is enabled (when building for tests) we also enable all that testutils generated code.
This issue has become more apparent to me lately because I'm experimenting with Soroban contracts inside Jupyter notebooks, as one way that folks can experiment and learn Soroban.
The Rust runtime kernel for Jupyter notebooks doesn't currently support enable features for code in the notebook so I end up with an odd situation where testutils are enabled in the SDK, but not in the contract.
After looking at how to solve this problem it occurred to me that the way we implemented the generated testutils code is not ideal, even independent of this problem.
Instead of implementing the generated code so it contains a new testutils feature for the contract crate, we should have made all the generated testutils code enabled/disabled based on the SDKs feature flag.
The advantage of this is that in any workspace whenever you have testutils enabled on the SDK, all contracts will also have testutils enabled. Instead of having many levers to pull to get testutils enabled, there is just one.
Reviewing
I recommend reviewing with whitespace changes disabled, because a large portion of the diff is the change to indents when some code was moved inside if..else statements.
Merging
This change should be practically non-breaking. While technically it is removing code from being generated when compiled with an SDK without testutils enabled, most of the testutils code in generated code would be unusable without it anyway.
It might be warranted to hold this change until v22, just for the purpose of shipping it in a preview / rc and getting feedback before committing to it. But I think the change is also pretty straightforward and low risk.
I'd like to merge this in a minor release, but it warrants some discussion.