refactor out namada crate #3402

tzemanovic · 2024-06-11T21:45:41Z

Describe your changes

Related to #2111 this PR refactors out the namada crate that will be removed. Two new crates are added:

namada_vp with native VP interfaces and VP host fns implementations
namada_vm with the VM and its wasm implementation

The native VPs from namada crate are moved to their respective crates using the approach outlined in https://hackmd.io/@heliax/r131cSMSR (note that existing cross system deps are not yet removed, only the ones in the moved native VPs):

governance (uses proof_of_stake::Read storage and token::Keys)
ethereum_bridge (uses token::Keys)
ibc (uses governance::Read, parameters::Keys + Read, token::Keys + Write and proof_of_stake::Read)
masp (uses governance::Read, parameters::Read) - moved into shielded_token
multitoken (uses governance::Read, parameters::Read) - moved into trans_token
parameters (uses governance::Read)
pgf
proof_of_stake (uses governance::Read)

Additionally:

dependency on namada_parameters crate in namada_state was removed with the same DI approach as above to allow the namada_parameters to depend on namada_vp to implement its native VP
mod namada::ledger::protocol is moved into namada_node::protocol
mod namada::vm::prefix_iter moved to namada_state::prefix_iter
fn dry_run_tx is moved into the node crate
mod control_flow has been moved from namada_sdk to namada_core (feature-guarded by non-default "control_flow" as it needs tokio)

Deps graph

The two highlighted crates are newly added:

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

v0.41.0

for reviews the most notable changes are in
- new traits for systems in core
- native VP implementations using these traits moved from namada crate into relevant system's crate
- new crates/sdk/src/validation.rs that injects the dependencies for native VPs

Checklist before merging to `draft`

I have added a changelog
Git history is in acceptable state

codecov · 2024-06-24T11:20:00Z

Codecov Report

Attention: Patch coverage is 59.05870% with 809 lines in your changes missing coverage. Please review.

Project coverage is 54.51%. Comparing base (1e21ae8) to head (1154023).

Files	Patch %	Lines
crates/ibc/src/lib.rs	14.72%	330 Missing ⚠️
crates/shielded_token/src/vp.rs	0.00%	53 Missing ⚠️
crates/node/src/shell/testing/node.rs	0.00%	48 Missing ⚠️
crates/parameters/src/vp.rs	0.00%	43 Missing ⚠️
crates/vm/src/wasm/run.rs	2.27%	43 Missing ⚠️
crates/apps_lib/src/client/rpc.rs	0.00%	36 Missing ⚠️
crates/node/src/bench_utils.rs	0.00%	35 Missing ⚠️
crates/node/src/dry_run_tx.rs	75.65%	28 Missing ⚠️
crates/node/src/protocol.rs	60.56%	28 Missing ⚠️
crates/apps_lib/src/client/tx.rs	0.00%	21 Missing ⚠️
... and 30 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3402      +/-   ##
==========================================
+ Coverage   53.66%   54.51%   +0.85%     
==========================================
  Files         322      323       +1     
  Lines      111871   113288    +1417     
==========================================
+ Hits        60035    61762    +1727     
+ Misses      51836    51526     -310

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sug0

nice changes! I have a couple of suggestions

sug0 · 2024-06-28T08:20:25Z

crates/benches/native_vps.rs

        // Use an empty verifiers set placeholder for validation, this is only
        // needed in actual txs to addresses whose VPs should be triggered
        let verifiers = Rc::new(RefCell::new(BTreeSet::<Address>::new()));

-        let exec_ctx = PseudoExecutionContext::new(ibc.ctx.pre());
+        let exec_ctx =
+            PseudoExecutionContext::<'_, '_, _, _, _, token::Store<_>>::new(


:o big turbo fish

I've added type aliases for this to improve this 5513de2

sug0 · 2024-06-28T08:23:10Z

crates/core/src/ledger/mod.rs

this file is empty but not deleted. we should git rm it

never trust what github says lol: fatal: pathspec 'crates/core/src/ledger/mod.rs' did not match any files . There were however some leftover regression files that are no longer being used, cleared in c0d04cb

sug0 · 2024-06-28T08:25:22Z

crates/core/src/parameters.rs

+
+/// Abstract parameters storage read interface
+pub trait Read<S> {
+    /// Storage error
+    type Err;


nice approach. I think we should eventually still try to hide these details from core, if possible, but it's a big improvement over the previous iteration of namada

yeah, I was thinking the same - maybe we could put these into a new crate namada_systems or something - It would actually help to have a concrete type for the error too to avoid having to type Err = namada_state::StorageError in some places

I've done this in a follow-up PR #3472

sug0 · 2024-06-28T08:30:44Z

crates/ethereum_bridge/src/vp/bridge_pool_vp.rs

 where
-    S: StateRead,
-    CA: 'static + WasmCacheAccess,
+    S: 'static + StateRead,
+    EVAL: 'static + VpEvaluator<'ctx, S, CA, EVAL>,
 {


not a fan of adding trait bounds to struct definitions. if possible, we should arrange to get rid of them everywhere in the codebase. they place unnecessary restrictions on these types, that could be made stricter at the method definition level using where clauses.

agree - we need to remove the bounds on the inner fields (in here it's the native_vp::Ctx) - I'll open a separate issue for this

sug0 · 2024-06-28T08:59:31Z

crates/ibc/src/vp/mod.rs

+    /// Parameters type
+    pub params: PhantomData<Params>,
+    /// Governance type
+    pub gov: PhantomData<Gov>,
+    /// Token type
+    pub token: PhantomData<Token>,
+    /// PoS type
+    pub pos: PhantomData<PoS>,


personal preference, I prefer creating a single type with all the generics using a tuple, eg

Suggested change

/// Parameters type

pub params: PhantomData<Params>,

/// Governance type

pub gov: PhantomData<Gov>,

/// Token type

pub token: PhantomData<Token>,

/// PoS type

pub pos: PhantomData<PoS>,

_marker: PhantomData<(Params, Gov, Token, PoS)>,

good call - done in 9695d7b

sug0 · 2024-06-28T09:05:41Z

crates/node/src/shell/queries.rs

        // Invoke the root RPC handler - returns borsh-encoded data on success
        let result = if query.path == RPC.shell().dry_run_tx_path() {
-            dry_run_tx(ctx, &query)
+            dry_run_tx(
+                unsafe { self.state.read_only().with_static_temp_write_log() },


Suggested change

unsafe { self.state.read_only().with_static_temp_write_log() },

// SAFETY: blah blah

unsafe { self.state.read_only().with_static_temp_write_log() },

consider adding a small explanation on why this is fine

sug0 · 2024-06-28T09:06:17Z

crates/node/src/shell/testing/node.rs

+                unsafe {
+                    borrowed.state.read_only().with_static_temp_write_log()
+                },


same thing here

sug0 · 2024-06-28T09:09:36Z

crates/sdk/src/migrations.rs

@@ -564,7 +544,6 @@ impl TypeHash
        typehash!(SerializeWrapper<BTreeMap<String, Address>>);
 }

-#[cfg(feature = "migrations")]


the sdk always has migrations enabled by default?

it's still guarded but I just moved it onto the mod migrations instead

sug0 · 2024-06-28T09:11:46Z

crates/state/src/wl_state.rs

+    /// The lifetime of borrows is unsafely extended to `'static` to allow usage
+    /// in node's `dry_run_tx`, which needs a static lifetime to be able to call
+    /// protocol's API that is generic over the state with a bound `S: 'static +
+    /// State` with a mutable reference to this struct.
+    /// Because the lifetime of `S` is invariant w.r.t. `&mut S`
+    /// (<https://doc.rust-lang.org/nomicon/subtyping.html>) we are faking a
+    /// static lifetime of `S` for `TempWlState`. This should be safe as neither
+    /// `db` nor `in_mem` are actually mutable, only the `write_log` which is
+    /// owned by this struct.


I think this is the explanation that should go under those unsafe blocks. here we should only write something like "the caller must guarantee that ..."

improved this in a04ad35

sug0 · 2024-06-28T09:15:33Z

crates/vm/src/wasm/run.rs

+                tracing::warn!(
+                    "VP eval from a native VP failed with:
+            {err}",
+                );


nit: fix this weird formatting

tzemanovic · 2024-07-09T15:34:15Z

rebased onto 0.40 and added ~~72b2282~~ ~~835061e~~ ~~4e5268b~~ a7b48c1 to move IBC impl out of the MASP VP

tzemanovic · 2024-07-10T08:49:42Z

@sug0 pls re-check this after the rebase - there's a new commit for MASP VP IBC dep a7b48c1

crates/ibc/src/lib.rs

mergify · 2024-07-30T10:50:29Z

Hey @tzemanovic, your pull request has been dequeued due to the following reason: PR_DEQUEUED.
Sorry about that, but you can requeue the PR by using @mergifyio requeue if you think this was a mistake.

tzemanovic · 2024-07-30T11:23:49Z

@Mergifyio requeue

mergify · 2024-07-30T11:24:01Z

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

mergify · 2024-07-30T11:25:33Z

Hey @tzemanovic, your pull request has been dequeued due to the following reason: PR_DEQUEUED.
Sorry about that, but you can requeue the PR by using @mergifyio requeue if you think this was a mistake.

Fraccaman · 2024-07-30T11:27:08Z

@Mergifyio refresh

mergify · 2024-07-30T11:27:11Z

refresh

✅ Pull request refreshed

mergify · 2024-07-30T11:28:44Z

Hey @tzemanovic, your pull request has been dequeued due to the following reason: PR_DEQUEUED.
Sorry about that, but you can requeue the PR by using @mergifyio requeue if you think this was a mistake.

Fraccaman · 2024-07-30T11:35:43Z

@Mergifyio refresh

mergify · 2024-07-30T11:35:48Z

refresh

✅ Pull request refreshed

Fraccaman · 2024-07-30T11:37:07Z

@Mergifyio refresh

mergify · 2024-07-30T11:37:13Z

refresh

✅ Pull request refreshed

tzemanovic force-pushed the tomas/refactor-out-namada-crate branch 8 times, most recently from c6ffa27 to 8dd4a05 Compare June 17, 2024 18:12

tzemanovic force-pushed the tomas/refactor-out-namada-crate branch from 8dd4a05 to 8e91b53 Compare June 21, 2024 14:53

tzemanovic mentioned this pull request Jun 24, 2024

chore: switch to use namada_sdk instead of namada anoma/namada-interface#890

Merged

tzemanovic force-pushed the tomas/refactor-out-namada-crate branch from 635d40b to 949f6b1 Compare June 25, 2024 12:55

tzemanovic marked this pull request as ready for review June 25, 2024 15:11

tzemanovic requested a review from sug0 June 25, 2024 15:59

tzemanovic added a commit that referenced this pull request Jun 27, 2024

changelog: add #3402

769e297

sug0 requested changes Jun 28, 2024

View reviewed changes

This was referenced Jun 28, 2024

DI - shielded token #3466

Merged

systems crate #3472

Merged

tzemanovic added a commit that referenced this pull request Jul 9, 2024

changelog: add #3402

81e16b5

tzemanovic force-pushed the tomas/refactor-out-namada-crate branch from 59918ef to 81e16b5 Compare July 9, 2024 15:31

tzemanovic added a commit that referenced this pull request Jul 9, 2024

changelog: add #3402

1104095

tzemanovic force-pushed the tomas/refactor-out-namada-crate branch from 81e16b5 to 1104095 Compare July 9, 2024 15:39

tzemanovic added a commit that referenced this pull request Jul 9, 2024

changelog: add #3402

d222183

tzemanovic force-pushed the tomas/refactor-out-namada-crate branch from 1104095 to d222183 Compare July 9, 2024 15:59

tzemanovic added a commit that referenced this pull request Jul 9, 2024

changelog: add #3402

c602ac1

tzemanovic force-pushed the tomas/refactor-out-namada-crate branch from d222183 to c602ac1 Compare July 9, 2024 19:53

sug0 previously approved these changes Jul 11, 2024

View reviewed changes

crates/ibc/src/lib.rs Outdated Show resolved Hide resolved

crates/ibc/src/lib.rs Outdated Show resolved Hide resolved

crates/ibc/src/lib.rs Outdated Show resolved Hide resolved

tzemanovic added a commit that referenced this pull request Jul 11, 2024

changelog: add #3402

82e0611

tzemanovic added 16 commits July 30, 2024 11:13

mv crates/namada/src/ledger/pos/vp.rs crates/proof_of_stake/src/vp.rs

6378391

proof_of_stake: fix VP post move

764ca00

namada: remove moved modules

343786f

mv crates/namada/src/ledger/mod.rs crates/node/src/dry_run_tx.rs

306e25f

rm -r crates/namada

cff91fc

vm + vp: add common crate attrs

0e685f5

remove all dependencies on namada crate

28b38c4

node: export dry_run_tx fn

50a9522

mv crates/sdk/src/control_flow/ crates/core/src/

cc56273

core: fix and feature guard control_flow after move from sdk

32ed67f

sdk: add native VPs with filled-in concrete system types

03c43ba

fix everything that was depending on rm'd namada crate

3238624

wasm/Make: run clippy with --all-targets

2895668

shielded_token/vp: replace IBC dep with DI

7bdd949

make check-crates: replace hard-coded args with the global list

05992de

changelog: add #3402

1154023

tzemanovic force-pushed the tomas/refactor-out-namada-crate branch from d08a0ea to 1154023 Compare July 30, 2024 10:13

mergify bot merged commit 63e81bb into main Jul 30, 2024
21 checks passed

mergify bot deleted the tomas/refactor-out-namada-crate branch July 30, 2024 11:38

	unsafe { self.state.read_only().with_static_temp_write_log() },
	// SAFETY: blah blah
	unsafe { self.state.read_only().with_static_temp_write_log() },

refactor out namada crate #3402

refactor out namada crate #3402

Conversation

tzemanovic commented Jun 11, 2024 • edited Loading

Describe your changes

Deps graph

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

Checklist before merging to draft

codecov bot commented Jun 24, 2024 • edited Loading

Codecov Report

sug0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tzemanovic commented Jul 9, 2024 • edited Loading

tzemanovic commented Jul 10, 2024

mergify bot commented Jul 30, 2024

tzemanovic commented Jul 30, 2024

mergify bot commented Jul 30, 2024

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

mergify bot commented Jul 30, 2024

Fraccaman commented Jul 30, 2024

mergify bot commented Jul 30, 2024

✅ Pull request refreshed

mergify bot commented Jul 30, 2024

Fraccaman commented Jul 30, 2024

mergify bot commented Jul 30, 2024

✅ Pull request refreshed

Fraccaman commented Jul 30, 2024

mergify bot commented Jul 30, 2024

✅ Pull request refreshed

tzemanovic commented Jun 11, 2024 •

edited

Loading

Checklist before merging to `draft`

codecov bot commented Jun 24, 2024 •

edited

Loading

tzemanovic commented Jul 9, 2024 •

edited

Loading