Skip to content

Conversation

Jiloc
Copy link
Contributor

@Jiloc Jiloc commented Sep 23, 2025

Description

This PR builds on top of #6532 and will stay in draft until that one will be merged.

Applicable issues

Additional info (benefits, drawbacks, caveats)

Checklist

  • Test coverage for new or modified code paths
  • Changelog is updated
  • Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
  • New clarity functions have corresponding PR in clarity-benchmarking repo
  • New integration test(s) added to bitcoin-tests.yml

@Jiloc Jiloc added this to the 3.2.0.0.2 milestone Sep 23, 2025
@Jiloc Jiloc self-assigned this Sep 23, 2025
@Jiloc Jiloc moved this to Status: In Review in Stacks Core Eng Sep 23, 2025
@Jiloc Jiloc moved this from Status: In Review to Status: 💻 In Progress in Stacks Core Eng Sep 23, 2025
@Jiloc Jiloc linked an issue Sep 24, 2025 that may be closed by this pull request
@jcnelson
Copy link
Member

I notice that the integration tests use clarity_cli. To me, this means that clarity_cli does not belong in contrib/, since that's where we put software that has no definitive level of support.

  • Do we intend to drop or limit support for clarity_cli, and thereby justify its removal to contrib/? I personally find myself using it on an almost daily basis, so I'm happy to be the code owner

  • Is there a way to separate out the vm_execute() function from clarity_cli that the integration tests need?

@Jiloc
Copy link
Contributor Author

Jiloc commented Sep 25, 2025

I notice that the integration tests use clarity_cli. To me, this means that clarity_cli does not belong in contrib/, since that's where we put software that has no definitive level of support.

  • Do we intend to drop or limit support for clarity_cli, and thereby justify its removal to contrib/? I personally find myself using it on an almost daily basis, so I'm happy to be the code owner
  • Is there a way to separate out the vm_execute() function from clarity_cli that the integration tests need?

Thanks for bringing this up! The move to contrib/ is purely for bug bounty scope reduction. We discovered CLI utilities (e.g. stacks-events, that was even broken!) were inadvertently included in our ImmuneFi bounty as part of stackslib. No intention to drop support for clarity-cli at all, just to move it to a less critical place.

Regarding the dependency, I checked it and vm_execute was just almost exaclty a duplication of execute_with_parameters_and_call_in_global_context from clarity. Probably it was duplicated because the original function is defined behind the "testing" feature flag. I refactored the code in the stacks-core tests to remove clarity-cli as a dependency and to directly use the function from clarity.

#[cfg(any(test, feature = "testing"))]
pub fn execute_with_parameters_and_call_in_global_context<F>(
program: &str,
clarity_version: ClarityVersion,
epoch: StacksEpochId,
use_mainnet: bool,
mut global_context_function: F,
) -> Result<Option<Value>>
where
F: FnMut(&mut GlobalContext) -> Result<()>,
{
use crate::vm::database::MemoryBackingStore;
use crate::vm::tests::test_only_mainnet_to_chain_id;
use crate::vm::types::QualifiedContractIdentifier;
let contract_id = QualifiedContractIdentifier::transient();
let mut contract_context = ContractContext::new(contract_id.clone(), clarity_version);
let mut marf = MemoryBackingStore::new();
let conn = marf.as_clarity_db();
let chain_id = test_only_mainnet_to_chain_id(use_mainnet);
let mut global_context = GlobalContext::new(
use_mainnet,
chain_id,
conn,
LimitedCostTracker::new_free(),
epoch,
);
global_context.execute(|g| {
global_context_function(g)?;
let parsed =
ast::build_ast(&contract_id, program, &mut (), clarity_version, epoch)?.expressions;
eval_all(&parsed, &mut contract_context, g, None)
})
}
#[cfg(any(test, feature = "testing"))]
pub fn execute_with_parameters(
program: &str,
clarity_version: ClarityVersion,
epoch: StacksEpochId,
use_mainnet: bool,
) -> Result<Option<Value>> {
execute_with_parameters_and_call_in_global_context(
program,
clarity_version,
epoch,
use_mainnet,
|_| Ok(()),
)
}

Does this approach make sense, or do you have a better idea?

@Jiloc Jiloc marked this pull request as ready for review September 25, 2025 14:49
@Jiloc Jiloc requested review from a team as code owners September 25, 2025 14:49
@Jiloc Jiloc moved this from Status: 💻 In Progress to Status: In Review in Stacks Core Eng Sep 25, 2025
Copy link
Contributor

@jferrant jferrant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Status: In Review
Development

Successfully merging this pull request may close these issues.

[chore] move helper binaries to contrib folder
3 participants