Skip to content
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

[aptos-vm] init_module & state value metadata for V2 loader #14341

Merged
merged 3 commits into from
Sep 5, 2024

Conversation

georgemitenkov
Copy link
Contributor

@georgemitenkov georgemitenkov commented Aug 19, 2024

Description

Two changes:

  1. Beautiful hack thanks to @zekun000 to support init module without index struct name cache flushing.
  2. Added API for AptosModuleStorage to query state value metadata.
    Note: tests 2will come when things are connected e2e.

Kept them as a separate PR for simplicity. This way it is easier to address all TODO(loader_v2)s...

Type of Change

  • New feature
  • Bug fix
  • Breaking change
  • Performance improvement
  • Refactoring
  • Dependency update
  • Documentation update
  • Tests

Which Components or Systems Does This Change Impact?

  • Validator Node
  • Full Node (API, Indexer, etc.)
  • Move/Aptos Virtual Machine
  • Aptos Framework
  • Aptos CLI/SDK
  • Developer Infrastructure
  • Other (specify)

How Has This Been Tested?

Key Areas to Review

Checklist

  • I have read and followed the CONTRIBUTING doc
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I identified and added all stakeholders and component owners affected by this change as reviewers
  • I tested both happy and unhappy path of the functionality
  • I have made corresponding changes to the documentation

Copy link

trunk-io bot commented Aug 19, 2024

⏱️ 2h 53m total CI duration on this PR
Job Cumulative Duration Recent Runs
general-lints 19m 🟩🟩🟩🟩🟩 (+6 more)
rust-cargo-deny 19m 🟩🟩🟩🟩🟩 (+6 more)
rust-move-tests 14m 🟥
rust-move-tests 14m 🟥
rust-move-tests 14m 🟥
rust-move-tests 13m 🟥
rust-move-tests 13m 🟥
rust-move-tests 13m 🟥
rust-move-tests 13m 🟥
check-dynamic-deps 13m 🟩🟩🟩🟩🟩 (+6 more)
rust-move-tests 9m 🟥
rust-move-tests 4m 🟥
semgrep/ci 4m 🟩🟩🟩🟩🟩 (+6 more)
rust-move-tests 3m 🟥
file_change_determinator 2m 🟩🟩🟩🟩🟩 (+6 more)
file_change_determinator 2m 🟩🟩🟩🟩🟩 (+6 more)
rust-move-tests 2m
permission-check 39s 🟩🟩🟩🟩🟩 (+6 more)
permission-check 35s 🟩🟩🟩🟩🟩 (+6 more)
permission-check 30s 🟩🟩🟩🟩🟩 (+6 more)
permission-check 29s 🟩🟩🟩🟩🟩 (+6 more)

settingsfeedbackdocs ⋅ learn more about trunk.io

Copy link
Contributor Author

georgemitenkov commented Aug 19, 2024

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @georgemitenkov and the rest of your teammates on Graphite Graphite

@georgemitenkov georgemitenkov changed the title [aptos-vm] init_module hack to avoid cache flushing [aptos-vm] init_module & state value metadata for V2 loader Aug 19, 2024
@georgemitenkov georgemitenkov marked this pull request as ready for review August 19, 2024 23:07
@georgemitenkov georgemitenkov requested review from runtian-zhou and ziaptos and removed request for davidiw and wrwg August 19, 2024 23:08
/// Returns the state value metadata of an associated with this module. The
/// error is returned if there is a storage error. If the module does not exist,
/// `None` is returned.
fn fetch_state_value_metadata(
Copy link
Contributor

Choose a reason for hiding this comment

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

how do we make sure to cleanly separate the functionality and apis vs the executor view traits? do the module interfaces just move here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! ModuleView is no longer needed, we have all the traits here, so this essentially merges executor traits into here. The reason is that we do not really need an adapter for modules, and we want to have a separate flow from data ideally. Block-STM will implement AptosModuleStorage directly.

Block-STM is generic, but I also previously added a trait method to convert addresses, module names into generic K: ModulePath (state keys) so we are good there as well.

@georgemitenkov
Copy link
Contributor Author

@gelash merging this because init module here is not correct anyway, and metadata is trivial + added tests and addressed your comments.

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