Fix fendermint compilation issues #499

cryptoAtwill · 2023-12-21T13:53:48Z

Fix fendermint compilation issues, i.e. clippy and also format.

Currently tests are still breaking:

Caused by:
    failed to execute contract call to f410forjzm4nb2ly4r4qaqjv2xjtfc6pvhinxl5py4zi:
    code: 33
    error: <no data; potential ABI mismatch>
    info: message failed with backtrace:
    00: f065 (method 3844450837) -- contract reverted (33)
    01: f065 (method 6) -- contract reverted (33)
    02: f010 (method 2) -- send aborted with code 33 (33)
    03: f01 (method 3) -- constructor failed: send aborted with code 33 (33)
    04: f0120 (method 1) -- constructor reverted (33)
    ', fendermint/fendermint/testing/contract-test/tests/staking/machine.rs:130:14

Will fix this in follow up PR.

aakoshh · 2023-12-21T18:25:52Z

fendermint/fendermint/vm/interpreter/src/fvm/state/ipc.rs

+    checkpoint_router: ContractCaller<
+        DB,
+        CheckpointingFacet<MockProvider>,
+        checkpointing_facet::CheckpointingFacetErrors,
+    >,
+    topdown_router: ContractCaller<
+        DB,
+        TopDownFinalityFacet<MockProvider>,
+        top_down_finality_facet::TopDownFinalityFacetErrors,
+    >,
+    xnet_messaging_facet: ContractCaller<
+        DB,
+        XnetMessagingFacet<MockProvider>,
+        xnet_messaging_facet::XnetMessagingFacetErrors,
+    >,


The naming convention was WhateverFacet became whatever field, so I think these should be called checkpointing, topdown and xnet_messaging. I can't see why it's checkpoint_router and topdown_router as "router" does not appear in the type, and all of them are facets so _facet in the third is redundant.

aakoshh · 2023-12-21T18:30:16Z

ipc/ipc/sdk/src/subnet.rs

-#[derive(Clone, Debug, Serialize_tuple, Deserialize_tuple, PartialEq, Eq)]
+#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq)]


Why change these now?

we are supporting lotus related now right? Serialize_tuple produces a different result than Serialize, just be consistent with other serialization methods.

Yeah, I think they chose this in some cases to save on the field names. I think conceptually it's like {foo: 1, bar: "hello"} vs [1, "hello"].

I'm a bit confused because these SDK-level structs shouldn't need any kind of serialization? They are just convenience types for decoupling the contract ABIs from the application-level, right? The data from these structs is pulled into the binding-generated ABI types in create_subnet, but never serialized in this form?

Bottom line: you may be able to remove the serde derive altogether here.

It's a bit legacy code as previously it's used in lotus.

You mean that these types in the past were previously serialized to feed them into legacy or Wasm actors, right? In that case, they would've been tuple-encoded. However, I suggest you simply drop the serde derivation here all together.

* CI: Build ABI for clippy * CI: Build ABI in workflow

raulk · 2023-12-22T11:17:43Z

contracts/.gitignore

@@ -6,6 +6,7 @@ crytic-export/
 .env
 broadcast/
 out/
+binding/src


@aakoshh When contracts/bindings change and a dev is adjusting the SDK and other downstream components, is the expectation that they'll locally generate the bindings and adapt the dependent code, but only check in the latter and not the former? Then the CI build would generate these before trying to build and would push the updated bindings?

This is up for debate, but currently I tried to follow what you said about not checking in generated code at all. So you are expected to update the SDK and Fendermint, check in those changes, but not check in the Ethers generated bindings, and CI will build it but also not push it either, it will never appear on Github.

That said, if it's useful to be able to share links to the Rust code, we can check it in. The reason I didn't do it is because people reported it changes for no reason.

However today I noticed that binding/src/lib.rs changes in a predictable way: when @cryptoAtwill pushes the modules are in alphabetical order; when I push they are in the order of appearance in build.rs. So I think there is something that formats the generated code on his end, it's not the fault of ethers being different.

raulk · 2023-12-22T11:19:31Z

fendermint/fendermint/testing/contract-test/tests/staking/machine.rs

-            permissioned: false,
+            permission_mode: 0, // collateral based
+            supply_source: ipc_actors_abis::register_subnet_facet::SupplySource {
+                kind: 0, // native token


Can we use an enum entry here instead?

* initial investigation * remove unused print * remove unused print * Update subnet activation logic (#502) * initial attempt * FIX: Activation * FIX: Restrict to root * FIX: Can checkpoint always * FIX: Skip join if staked * FIX: Can't checkpoint without validators * Update machine.rs FIX: Clippy * Update Makefile FIX: Docker deps * Update lib.rs FIX: contract path * fix library_dependencies test. * FIX: New ABI in test * Non-alphabetical order * FIX: Field names * FIX: Fix docs after u64 became U256 --------- Co-authored-by: Akosh Farkash <aakoshh@gmail.com> Co-authored-by: raulk <raul.kripalani@gmail.com> --------- Co-authored-by: Akosh Farkash <aakoshh@gmail.com> Co-authored-by: raulk <raul.kripalani@gmail.com>

fix compilation

e3943c0

cryptoAtwill requested review from aakoshh, raulk and snissn December 21, 2023 13:53

aakoshh reviewed Dec 21, 2023

View reviewed changes

aakoshh added 2 commits December 21, 2023 18:35

CI: Build ABI for clippy (#500)

cd6702b

* CI: Build ABI for clippy * CI: Build ABI in workflow

Merge branch 'main' into fix-fendermint

0abb282

aakoshh force-pushed the fix-fendermint branch from 728ed53 to 0abb282 Compare December 21, 2023 18:36

aakoshh added 6 commits December 21, 2023 18:59

FIX: Lint

7e0d4ac

GEN: Bindings

a399fff

BUILD: Only run fmt and clippy on own packages

056e389

Merge branch 'main' into fix-fendermint

b0961ff

FIX: Extra nesting in docker build

e5fe4f1

FIX: Remove .out

9fd435c

aakoshh force-pushed the fix-fendermint branch from 38543b5 to d171d6c Compare December 21, 2023 20:07

BUILD: Remove binding/src from source control

5b54e1b

aakoshh force-pushed the fix-fendermint branch from d171d6c to 5b54e1b Compare December 21, 2023 20:14

aakoshh added 5 commits December 21, 2023 20:43

FIX: e2e test paths

c34c71a

FIX: Remove --min-collateral

2ef3cbe

FIX: Missed a path

0b168e7

BUILD: Install cargo-make if needed

d3ffddf

FIX: Bundle path

d4c5205

raulk reviewed Dec 22, 2023

View reviewed changes

aakoshh merged commit b569f87 into update-ipc-and-solidity Dec 22, 2023
7 checks passed

aakoshh deleted the fix-fendermint branch December 22, 2023 18:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix fendermint compilation issues #499

Fix fendermint compilation issues #499

cryptoAtwill commented Dec 21, 2023

aakoshh Dec 21, 2023 •

edited

Loading

aakoshh Dec 21, 2023

cryptoAtwill Dec 22, 2023

aakoshh Dec 22, 2023

raulk Dec 22, 2023

raulk Dec 22, 2023

cryptoAtwill Dec 22, 2023

raulk Dec 22, 2023

raulk Dec 22, 2023

aakoshh Dec 22, 2023

raulk Dec 22, 2023

		#[derive(Clone, Debug, Serialize_tuple, Deserialize_tuple, PartialEq, Eq)]
		#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq)]

Fix fendermint compilation issues #499

Fix fendermint compilation issues #499

Conversation

cryptoAtwill commented Dec 21, 2023

aakoshh Dec 21, 2023 • edited Loading

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

aakoshh Dec 21, 2023 •

edited

Loading