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

Sorting modules by dependencies #549

Merged
merged 34 commits into from
Jul 27, 2023
Merged

Conversation

LukaszRozmej
Copy link
Contributor

@LukaszRozmej LukaszRozmej commented Jul 24, 2023

Description

  • ModuleInfo trait was expanded with the dependencies function and returns the addresses of modules it depends on.
  • derive_module_info macro generates dependencies function for each module using #[module] attribute.
  • sort_by_dependencies function allows to sort ModuleInfo by their dependencies. It takes modules and additional type tuples and returns sorted vector of this additional type. This done this way to easily map it back to compile time generated constants (see expanded derive_genesis macro).
  • derive_genesis macro changed how fn genesis was created. Now it creates a Vec<ModuleTrait, usize> that is sorted with sort_by_dependencies and then the usize are matched to correct module genesis call.
  • Two errors were introduced:
  1. Missing module in runtime
  2. Cyclic dependency

Side effect changes

  • Addresses are now hashable. AddressTrait and all its implementations was expanded with Hash trait.
  • ModuleInfo is not subtrait of Default, this wasn't required and made the trait not object safe.

Example of expanded derive_genesis macro for demo-app

    impl<C: Context> sov_modules_api::Genesis for Runtime<C> {
        type Context = C;
        type Config = GenesisConfig<C>;
        fn genesis(
            &self,
            config: &Self::Config,
            working_set: &mut sov_state::WorkingSet<
                <<Self as sov_modules_api::Genesis>::Context as sov_modules_api::Spec>::Storage,
            >,
        ) -> core::result::Result<(), sov_modules_api::Error> {
            let modules: ::std::vec::Vec<
                (
                    &dyn ::sov_modules_api::ModuleInfo<
                        Context = <Self as sov_modules_api::Genesis>::Context,
                    >,
                    usize,
                ),
            > = <[_]>::into_vec(
                #[rustc_box]
                ::alloc::boxed::Box::new([
                    (&self.bank, 0usize),
                    (&self.sequencer_registry, 1usize),
                    (&self.election, 2usize),
                    (&self.value_setter, 3usize),
                    (&self.accounts, 4usize),
                ]),
            );
            let sorted_modules = ::sov_modules_api::sort_values_by_modules_dependencies(
                modules,
            )?;
            for module in sorted_modules {
                match module {
                    0usize => {
                        ::sov_modules_api::Genesis::genesis(
                            &self.bank,
                            &config.bank,
                            working_set,
                        )
                    }
                    1usize => {
                        ::sov_modules_api::Genesis::genesis(
                            &self.sequencer_registry,
                            &config.sequencer_registry,
                            working_set,
                        )
                    }
                    2usize => {
                        ::sov_modules_api::Genesis::genesis(
                            &self.election,
                            &config.election,
                            working_set,
                        )
                    }
                    3usize => {
                        ::sov_modules_api::Genesis::genesis(
                            &self.value_setter,
                            &config.value_setter,
                            working_set,
                        )
                    }
                    4usize => {
                        ::sov_modules_api::Genesis::genesis(
                            &self.accounts,
                            &config.accounts,
                            working_set,
                        )
                    }
                    _ => {
                        Err(
                            ::sov_modules_api::Error::ModuleError(
                                ::anyhow::Error::msg({
                                    let res = ::alloc::fmt::format(
                                        format_args!("Module not found: {0:?}", module),
                                    );
                                    res
                                }),
                            ),
                        )
                    }
                }?
            }
            Ok(())
        }
    }

Linked Issues

Testing

  • Unit test were added to cover derive_module_info macro and sort_by_dependencies, derive_genesis macro was already expanded in other tests.

Docs

  • No changes needed.

@LukaszRozmej LukaszRozmej marked this pull request as draft July 24, 2023 14:20
@LukaszRozmej LukaszRozmej changed the title WIP: Sorting modules by dependencies Sorting modules by dependencies Jul 25, 2023
@LukaszRozmej LukaszRozmej marked this pull request as ready for review July 25, 2023 09:14
Copy link
Member

@preston-evans98 preston-evans98 left a comment

Choose a reason for hiding this comment

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

Left a few minor comments, but this looks like it will get the job done. That being said, I'm curious about the approach of assigning each module a number and then matching on the sorted module numbers? I have two minor concerns there.

First, I think the generated code might be quadratic in the number of modules (I'm not sure if rustc is smart enough to optimize a match on an integer into a jump). That's probably ok, since the number of modules should be small, but it's a little unexpected.

Second, the match just feels a little bit ugly/hacky. Again that's not a blocker - sometimes you need a hack to make things work; but this seems like code smell to me.

I think we can solve both problems with a small tweak to the API. If we make sorted_modules a &mut Vec<&'a dyn Genesis>, then we can just iterate over it directly instead of using the match. That also means we'll need to change the signature of the other args to have a dyn Genesis in place of T. Or, equivalently, we can use a supertrait of ModuleInfo and Genesis so that we can pass around a single trait object.

Anyway, I'm curious if there's some drawback to that approach that I'm missing.

module-system/sov-modules-api/src/lib.rs Outdated Show resolved Hide resolved
module-system/sov-modules-api/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@preston-evans98 preston-evans98 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @LukaszRozmej

@codecov
Copy link

codecov bot commented Jul 27, 2023

Codecov Report

Merging #549 (250e97e) into nightly (d4b39e3) will increase coverage by 0.4%.
The diff coverage is 99.0%.

Files Changed Coverage Δ
adapters/celestia/src/celestia.rs 56.6% <0.0%> (ø)
adapters/celestia/src/verifier/address.rs 72.5% <0.0%> (ø)
module-system/sov-modules-api/src/lib.rs 84.2% <100.0%> (+22.8%) ⬆️
module-system/sov-modules-api/src/tests.rs 100.0% <100.0%> (ø)
...-system/sov-modules-macros/src/dispatch/genesis.rs 100.0% <100.0%> (ø)
...odule-system/sov-modules-macros/src/module_info.rs 88.1% <100.0%> (+0.9%) ⬆️
rollup-interface/src/state_machine/mocks.rs 62.0% <100.0%> (ø)

... and 1 file with indirect coverage changes

@LukaszRozmej LukaszRozmej merged commit 80b2518 into nightly Jul 27, 2023
@LukaszRozmej LukaszRozmej deleted the lukasz/sorted_dependencies branch July 27, 2023 23:03
preston-evans98 pushed a commit that referenced this pull request Sep 14, 2023
* Generating ModuleTrait.dependencies and implementing sort_by_dependencies

* fix whitespace

* Add ModuleInfo macro dependencies test

* change sorting modules to have additional type

* change genesis macro

* add sorting modules tests

* formatting

* make Vec fully qualified in macros

* split sort_modules_by_dependencies and sort_values_by_modules_dependencies

* rename param

* fix whitespace

* refactor sort_modules_by_dependencies to use ModuleVisitor struct for mutable elements

* fix whitespace

* refactor

* Add support for cyclic dependencies

* Change sort_values_by_modules_dependencies from &Tvalue to TValue

* remove & in genesic macro identyfiers

* fix test

* fix

* fix test

* move Hash trait implementation for addresses to macros

* reverse cycle order in error message

* I give up, make test order invariant

* Try fixing test

* simplify tests

* improve cycle detection with stack

simplify test with mocking modules

* fix too much cloning

* slim tests setup
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.

Genesis cross-module dependencies.
4 participants