Skip to content

Add test-only code publish function #13174

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

alnoki
Copy link
Contributor

@alnoki alnoki commented May 2, 2024

Current problem

Currently the code module lacks test-only functions, making it difficult for devs to mock out or stub behaviors that depend on autonomous publication during runtime.

In particular, when a package is published via code::publish_package_txn(), an inner call to check_dependencies verifies that each of its dependencies have a code::PackageRegistry resource under their accounts:

assert!(exists<PackageRegistry>(dep.account), error::not_found(EPACKAGE_DEP_MISSING));

In the general case, the only way to ensure that a PackageRegistry exists under an account is to call code::publish_package_txn():

if (!exists<PackageRegistry>(addr)) {
move_to(owner, PackageRegistry { packages: vector::empty() })
};

Hence, to run a test that calls code::publish_package_txn(), there first needs to be setup that calls the same function for each of the expected dependencies. Problematically, however, this function relies on an inner call to the native_request_publish native function in code.rs, which errors out with EALREADY_REQUESTED when one attempts to publish two packages in the same txn:

// Request publish
if (features::code_dependency_check_enabled())
request_publish_with_allowed_deps(addr, module_names, allowed_deps, code, policy.policy)
else
// The new `request_publish_with_allowed_deps` has not yet rolled out, so call downwards
// compatible code.
request_publish(addr, module_names, code, policy.policy)

if code_context.requested_module_bundle.is_some() {
// Can't request second time.
return Err(SafeNativeError::Abort {
abort_code: EALREADY_REQUESTED,
});

Hence it is impossible to write unit tests for code that publishes a package with even a single dependency (because it is impossible to publish both the dependency and the relevant package during the same unit test, since native_request_publish throws EALREADY_REQUESTED).

For packages that have more than one dependency, it is impossible to publish even the dependencies (e.g. to ensure that a code::PackageRegistry exists under each relevant account) for the same reason.

Solution

  • This PR adds a test-only code publish function, code::publish_package_txn_for_testing() which circumvents the above hassle by simply ignoring the blocking native function call (which appears to mainly serve metering purposes and occurs after all other extensive validation)
  • Abstract out the publish func inner logic so that identical code (except for the offending native function call) is used both in tests and during runtime

This approach will allow developers to write unit tests for code that calls code::publish_package_txn()

Housekeeping

The following was run from repository root to ensure CI passes:

cargo build -p aptos-cached-packages
pre-commit run --all-files
scripts/rust_lint.sh
aptos move test --package-dir aptos-move/framework/aptos-framework/ --dev

@alnoki alnoki requested review from davidiw, movekevin and wrwg as code owners May 2, 2024 04:58
Copy link

trunk-io bot commented May 2, 2024

⏱️ 8s total CI duration on this PR

Job Cumulative Duration Recent Runs
permission-check 2s 🟥
permission-check 2s 🟥
permission-check 2s 🟥
permission-check 2s 🟥

settingsfeedbackdocs ⋅ learn more about trunk.io

@alnoki alnoki changed the title Add test-only code publish func Add test-only code publish function May 2, 2024
Copy link
Contributor

This issue is stale because it has been open 45 days with no activity. Remove the stale label, comment or push a commit - otherwise this will be closed in 15 days.

@github-actions github-actions bot added the Stale label Jun 17, 2024
@alnoki
Copy link
Contributor Author

alnoki commented Jun 17, 2024

Bumping this as the PR eases DevEx

@github-actions github-actions bot removed the Stale label Jun 18, 2024
Copy link
Contributor

github-actions bot commented Aug 2, 2024

This issue is stale because it has been open 45 days with no activity. Remove the stale label, comment or push a commit - otherwise this will be closed in 15 days.

@github-actions github-actions bot added the Stale label Aug 2, 2024
@alnoki
Copy link
Contributor Author

alnoki commented Aug 2, 2024

Bumping again as the PR eases DevEx

@github-actions github-actions bot removed the Stale label Aug 3, 2024
Copy link
Contributor

This issue is stale because it has been open 45 days with no activity. Remove the stale label, comment or push a commit - otherwise this will be closed in 15 days.

@github-actions github-actions bot added the Stale label Sep 18, 2024
@alnoki
Copy link
Contributor Author

alnoki commented Sep 18, 2024

Bumping per stale label

@github-actions github-actions bot removed the Stale label Sep 19, 2024
Copy link
Contributor

github-actions bot commented Nov 3, 2024

This issue is stale because it has been open 45 days with no activity. Remove the stale label, comment or push a commit - otherwise this will be closed in 15 days.

@github-actions github-actions bot added the Stale label Nov 3, 2024
@alnoki
Copy link
Contributor Author

alnoki commented Nov 4, 2024

Bumping per stale label

@github-actions github-actions bot removed the Stale label Nov 5, 2024
Copy link
Contributor

This issue is stale because it has been open 45 days with no activity. Remove the stale label, comment or push a commit - otherwise this will be closed in 15 days.

@github-actions github-actions bot added the Stale label Dec 20, 2024
@alnoki
Copy link
Contributor Author

alnoki commented Dec 20, 2024

Bumping per stale label

@github-actions github-actions bot removed the Stale label Dec 21, 2024
Copy link
Contributor

github-actions bot commented Mar 5, 2025

This issue is stale because it has been open 45 days with no activity. Remove the stale label, comment or push a commit - otherwise this will be closed in 15 days.

@github-actions github-actions bot added the Stale label Mar 5, 2025
@alnoki
Copy link
Contributor Author

alnoki commented Mar 5, 2025

Bumping per stale label

@github-actions github-actions bot removed the Stale label Mar 6, 2025
@gregnazario
Copy link
Contributor

@alnoki please rebase and fix merge conflicts

@alnoki alnoki force-pushed the publish-package-txn-for-testing branch from 96df9d1 to 72db6e5 Compare April 2, 2025 20:52
@alnoki alnoki force-pushed the publish-package-txn-for-testing branch from 72db6e5 to e088ab2 Compare April 2, 2025 21:13
@alnoki
Copy link
Contributor Author

alnoki commented Apr 2, 2025

@alnoki please rebase and fix merge conflicts

@gregnazario I've just rebased and resolved all merge conflicts - does this PR now require workflow approval?

Copy link
Contributor

This issue is stale because it has been open 45 days with no activity. Remove the stale label, comment or push a commit - otherwise this will be closed in 15 days.

@github-actions github-actions bot added the Stale label May 18, 2025
@alnoki
Copy link
Contributor Author

alnoki commented May 19, 2025

@gregnazario bumping the stale label per the requested cleanup I performed

@github-actions github-actions bot removed the Stale label May 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants