Modularization: core refactors by tzemanovic · Pull Request #2312 · namada-net/namada

tzemanovic · 2023-12-19T12:30:14Z

Describe your changes

We're dividing namada_core crate into more granular low-level crates. The updated dependency graph looks like this:

with dev-deps (blue):

This entails:

Add new crates from namada_core modules
Make namada_core compile
Make each new crate compile
Update all the existing crate to depend on the new crates
cargo check --tests on each crate until passing

Progress for "Make each new crate compile"

Low-level

Sub-systems

Other

Indicate on which release or other PRs this topic is based on

v0.29.0 - Diff range for review: https://github.com/anoma/namada/pull/2312/files

Checklist before merging to `draft`

I have added a changelog
Git history is in acceptable state

batconjurer · 2023-12-21T09:39:27Z

core/src/types/sign.rs

+use thiserror::Error;
+
+use super::address::Address;
+use super::key::common;


It might make sense to pub use here. If I were a new dev and saw this file, I would assume I could find keys and signature types in here.

batconjurer · 2023-12-21T09:42:50Z

core/src/types/token.rs

 //! A basic fungible token

 use std::cmp::Ordering;
+use std::collections::BTreeMap;


How do we decide what gets its own module and what stays under core? For example, why isn't this its own module or account not under core?

we should probably move this into the new trans_token crate - there are still few files left in core that depend on it, but they can also be moved out

sug0

So far I've looked at:

The pos_queries and eth_bridge_queries changes, and I like them.
The general layout of the repository. The root of the project looks too cluttered now, so I suggest we move non-wasm crates to a separate directory. (native?? ledger?? idk)
(Follow-up of point 2). This is a somewhat counterintuitive point of criticism to the objective at hand, but perhaps we have too many crates now with very little benefit (other than smaller wasm sizes, potentially). Our main goal should be unifying the APIs into an abstraction that will prevent code changes from bubbling up the dependency graph, wrecking havoc during merges, refactors, etc. As a first step to realize that goal, this is still an excellent effort!
The dependency graph. I noticed storage depended on tx, which I found odd, so I looked through the code to see where it got used, and left some suggestions to clean up that dependency from the graph.

I might revisit this PR later today.

sug0 · 2024-01-12T09:58:34Z

storage/src/tx_queue.rs

+/// A wrapper for `crate::types::transaction::WrapperTx` to conditionally
+/// add `has_valid_pow` flag for only used in testnets.
+#[derive(Debug, Clone, BorshDeserialize, BorshSerialize)]
+pub struct TxInQueue {
+    /// Wrapper tx
+    pub tx: Tx,
+    /// The available gas remaining for the inner tx (for gas accounting).
+    /// This allows for a more detailed logging about the gas used by the
+    /// wrapper and that used by the inner
+    pub gas: Gas,
+}


Seems like the only reason storage depends on tx currently is because it requires Tx in the tx_queue module; maybe the same could be said for namada_gas::Gas. Since tx_queue is re-exported to state, and only gets used in apps, and logically it is considered stateful data, why not move the tx_queue module to the state crate? This way we potentially remove two dependencies from storage.

Yeah, I actually had some issue with having these dependencies elsewhere. Moving it to state sounds like a good call

ah, unfortunately it's not so straight-forward as the BlockStateRead in storage/src/db.rs depends on it :(

vp_prelude/src/lib.rs

* origin/tomas+tiago/modularization: (118 commits) changelog: add #2312 add impl for VP get_pred_epochs pos_queries: remove unused methods fix masp storage key regression undo reintroduced typos fix `get_current_decision_height` by moving it to WlStorage vm_env: add `tx_get_pred_epoch` impl fix rustdocs macros: fix tests fix the shell test_queries fix TxGasMeter construction from GasLimit wasm: fix tests fix shared/sdk build without rand ethereum_bridge: fix build with "testing" feature various fixes tests: fix build benches: fix build apps: fix tests make -C wasm_for_tests rm ethereum_bridge dep from tx_prelude tidy up feature flags to avoid wasm-bindgen in wasm crates shared: fix tests sdk: fix tests ibc: fix tests ethereum_bridge: fix tests proof_of_stake: fix tests shielded_token: fix tests state: fix tests parameters: fix warning account: fix tests vote_ext: fix tests tx: fix tests gas: fix tests merkle_tree: fix tests rename struct Storage to State storage: fix tests move types, trait DB and MockDB impl from state to storage core: fix tests wasm: update lock file wasm_for_tests: fix build light_sdk: fix build encoding_spec: fix build apps: fix build shared: fix build wasm: fix build sdk: fix build token: add common `write_params` fn 0.29 rebase fixes add namada_token crate vp_prelude: fix build tx_prelude: fix build shielded_token: fix build tx_env: fix build ibc: fix build ethereum_bridge: fix build ethereum_bridge: use the original name for wrapped erc20s state: fix build merkle_tree: fix build vote_ext: fix build storage: refactor new fns in StorageRead using pred `Epochs` proof_of_stake: fix build rebase fixes merkle_tree: wip fix build proof_of_stake: wip fix build move ics23_specs from state into merkle_tree governance: fix build account: fix build macros: use full paths types used in `derive(StorageKeys)` trans_token: fix build tx: wip fix build move vote_ext tx data types from tx to vote_ext crate gas: fix build parameters: fix build move collection validation helpers into vp_env crate mv proto symlink to namada_tx move collection validation helpers from storage to vp_env storage: fix build core: re-export borsh move account related code from namada_storage to namada_account trans_token: fix build move ERC20 address constructors back to core core/types/internal: remove unsued import move core::types::vote_extensions into namada_vote_ext crate move core MembershipProof into merkle_tree crate move EvalVp from core into tx::data::eval_vp core/types/key: remove unused error case move state::traits hasher into core::types::hash namada_account: take account keys storage from namada_core::types::key namada_account: fix imports, update docstring add namada_account crate from namada_core::types::account move core::types::internal::tx into namada_state::tx_queue tx_env: update imports add namada_tx_env out of core::ledger::tx_env mod add namada_storage crate out of core::ledger::storage_api core: make vp_env crate out of ledger::vp_env mod core: move types::transaction mods into namada_tx::data update lock core: move proto and into namada_tx crate gas: fix imports core: move ledger::gas into new namada_gas crate ...

tzemanovic force-pushed the tomas+tiago/modularization branch from f4b077d to f7f1d86 Compare December 19, 2023 12:36

tzemanovic added the modularization label Dec 19, 2023

tzemanovic force-pushed the tomas+tiago/modularization branch from 06955dc to c4f79a6 Compare December 19, 2023 18:02

batconjurer reviewed Dec 21, 2023

View reviewed changes

tzemanovic force-pushed the tomas+tiago/modularization branch from 3a17751 to d0bd9b6 Compare December 21, 2023 17:24

tzemanovic added 24 commits January 3, 2024 12:34

create trans_token crate from storage_api/token mod

dad7b63

trans_token: add storage_key and move relevant fns from core

d2e240e

move core token storage reads to trans_token::storage

0ee2f6d

core: replace token type import

b103c27

core: separate gov code into namada_governance crate

1520ad5

governance: update import paths

74843e1

core: move pgf into namada_governance

c39544a

gov: join pgf storage mods

5d0c3b1

add namada_ibc crate

c800630

core: make namada_shielded_token crate out of masp mods

4cd888a

shielded_token: move ConversionState into core::types::token

cc0b7ea

move some governance types back into core

31e8b84

core: move eth_bridge storage mods to ethereum_bridge crate

0562841

core: move merkle_tree into namada_merkle_tree crate

ee76120

core: add namada_storage crate out of ledger::storage

a469ad6

core: add namada_parameters crate out of ledger::parameters

566fa6d

parameters: fix imports

1e04b0f

core: move validate_tx_bytes into parameters crate

642a4e4

core: move ledger::gas into new namada_gas crate

3686e56

gas: fix imports

dbef2de

core: move proto and into namada_tx crate

94d1ba9

update lock

20de0b7

core: move types::transaction mods into namada_tx::data

a4490d4

core: make vp_env crate out of ledger::vp_env mod

5ef1d70

sug0 requested changes Jan 12, 2024

View reviewed changes

tzemanovic commented Jan 12, 2024

View reviewed changes

vp_prelude/src/lib.rs Outdated Show resolved Hide resolved

tzemanovic mentioned this pull request Jan 12, 2024

move crates into crates dir #2386

Merged

2 tasks

tzemanovic added 19 commits January 12, 2024 17:32

make -C wasm_for_tests

653fd18

apps: fix tests

0fbf42f

benches: fix build

05adcfb

tests: fix build

eb454ce

various fixes

38570bb

ethereum_bridge: fix build with "testing" feature

f99eacb

fix shared/sdk build without rand

f980dbe

wasm: fix tests

f0f8c7f

fix TxGasMeter construction from GasLimit

a22001f

fix the shell test_queries

f9340f6

macros: fix tests

baa1d01

fix rustdocs

5124762

vm_env: add tx_get_pred_epoch impl

37fe7b6

fix get_current_decision_height by moving it to WlStorage

4101ec0

undo reintroduced typos

518358a

fix masp storage key regression

299dfb9

pos_queries: remove unused methods

721fef1

add impl for VP get_pred_epochs

d6bc44b

changelog: add #2312

21eddea

tzemanovic force-pushed the tomas+tiago/modularization branch from c1fb3d4 to d6bc44b Compare January 12, 2024 17:39

brentstone mentioned this pull request Jan 13, 2024

Draft 0.30.1 #2392

Closed

tzemanovic added a commit that referenced this pull request Jan 16, 2024

fixup! Merge branch 'tomas+tiago/modularization' (#2312)

14b80a0

tzemanovic added the merged to draft label Jan 16, 2024

tzemanovic merged commit a9a3a94 into main Jan 17, 2024

tzemanovic deleted the tomas+tiago/modularization branch January 17, 2024 16:34

cwgoes mentioned this pull request Feb 2, 2024

Restructure codebase for a module-based separation of concerns #2111

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modularization: core refactors#2312

Modularization: core refactors#2312
tzemanovic merged 118 commits intomainfrom
tomas+tiago/modularization

tzemanovic commented Dec 19, 2023 •

edited

Loading

Uh oh!

batconjurer Dec 21, 2023 •

edited

Loading

Uh oh!

batconjurer Dec 21, 2023

Uh oh!

tzemanovic Dec 21, 2023

Uh oh!

sug0 left a comment

Uh oh!

sug0 Jan 12, 2024

Uh oh!

tzemanovic Jan 12, 2024

Uh oh!

tzemanovic Jan 12, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

tzemanovic commented Dec 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe your changes

Progress for "Make each new crate compile"

Low-level

Sub-systems

Other

Indicate on which release or other PRs this topic is based on

Checklist before merging to draft

Uh oh!

batconjurer Dec 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

batconjurer Dec 21, 2023

Choose a reason for hiding this comment

Uh oh!

tzemanovic Dec 21, 2023

Choose a reason for hiding this comment

Uh oh!

sug0 left a comment

Choose a reason for hiding this comment

Uh oh!

sug0 Jan 12, 2024

Choose a reason for hiding this comment

Uh oh!

tzemanovic Jan 12, 2024

Choose a reason for hiding this comment

Uh oh!

tzemanovic Jan 12, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tzemanovic commented Dec 19, 2023 •

edited

Loading

Checklist before merging to `draft`

batconjurer Dec 21, 2023 •

edited

Loading