Small audit of Core crate, asking at every file: Do we really need this?

[Eikix]

Following Marie Kondo's method, let's ask ourselves for every file and design choices in the Core crate: "Does it bring me joy?"

Jokes aside. Let's quickly look at each file in Core crate and see if they make sense. Our context is that we want to perform a great cleaning in the codebase, ideally reduce brain twisters and unnecessary complexity. The best code is the one that isn't written.

Let's start by the contracts folder. It contains abstractions over smart contracts, exposing an intuitive and convenient API for us to call into methods of our contracts.

Core > Contracts

Erc20

The ethereum_erc20 abstraction is not needed in my opinion. It's an abstraction over the Ethereum world. For any such abstraction, we should use ether-rs or alloy-rs.
Current behaviour: this abstraction does: prepare kakarot call -> eth_call (balanceOf) -> return
Replacement: use a strongly-typed ether-rs Erc20 contract, pass it the eth-rpc, call balanceOf

The starknet_erc20 abstraction is unlucky in my opinion. We would ideally use starknet-rs and so-called strongly typed contract bindings. Let's investigate how complicated this is to achieve. I hear Glihm and Pia worked on starknet abigen.

Account

Akin to starknet_erc20, I'd rather have KakarotAccount and any so-called Accounts (EVM Accounts deployed by Kakarot as Starknet contracts) as strongly typed contract bindings.

Replacement: no replacement until we can use strongly typed contract bindings.

Contract Account

Same as above.

Kakarot

Same as above.

Testing strategy:

It seems we can achieve a good testing strategy on all those items by using a Katana test sequencer.
We're currently using a Testnet gateway sequencer for testing erc20 behaviour. We can stop doing that.

Core > Models

Balance

I think balance.rs can be deleted, I don't see the value it brings. Instead, its logic can be moved directly to eth-rpc.

Block ✅

Call ✅

Convertible ✅

Event Filter ✅

Event ✅

Felt ✅

Mod ⚠️

Surprising content of mod.rs, contains the error enum of the folder. Maybe refactor the error somewhere else.

Signature ✅

Tests ⚠️

Surprising content: only one const.

Transaction receipt ✅

Transaction ✅

Testing strategy

It appears these files rely on fixtures, i.e. file-based test data, operate conversion from there. These files have a starknet and eth variant, and attempt to convert, then assert on equality.
This can be fine, though it sounds very painful to setup. Using a test sequencer is not trivial either as:

• it would need to have preexisting blocks/fixtures that enable querying data, and the tester should still provide the target EVM converted format.

Ideally, test data should be easily maintainable.

Core > Mock

Foreword: this folder is very cursed. It took great pain to build it, and it's unclear if it's truly useful and protects us from bugs and regression.

Assert helpers

They are used a little bit randomly throughout the repository. I'd like to rethink these assert helpers, because they don't really make sense in our current use case.
For instance:

pub fn assert_block(block: &Rich<Block>, starknet_res: String, starknet_txs: String, hydrated: bool);

Why would we assert the equality of an ethereum rich block, and a starknet block? If we want to assert equality in our test's business logic, we should take a starknet block, attempt to convert it to an eth block (using model folder), and then assert the equality of the two ethereum blocks.

Following this logic, we should delete assert_helpers.

Constants

It's unclear why we have test constants in a folder that is not exclusively conditionally compiled in test situations. We need to delete those or mark them as only usable in tests.

Following this, we should only conditionally compile constants.rs. Additionally, we should check if each constant is useful.

Mock Starknet

mock_starknet.rs is the heart of the file-based test strategy of the core crate. It relies on the coupling of eth and starknet file fixtures used by the provider to "query" a static starknet (file based request solving).
This was very painful to set up, and might not really be useful.

I'd recommend removing it completely with a test sequencer based testing.

Serde

Unclear to me what this file does.

I recommend deleting it.

Core > Client

Config

This file is a bit unlucky, we created it because we needed to differentiate building a Sequencer (feeder gateway) based client, or a RPC provider based client.

My recommendation is to greatly simplify this added logic when:

• We implement simulateTransaction and estimateGas on the RPC provider and not the feeder gateway
• The feeder gateway is truly deprecated (circa. January 2024)
  By:
• Removing all differentiated logic between gateway and RPC provider (remove gateway-linked logic), only keep RPC provider.

Constants

These constants were added to, more than pruned. I recommend creating an issue for auditing this file alone. Is each constant useful? Should it be replaced for good by live data?

Errors

This file is fine, though most of the logic of Core > Client > mod.rs (formerly KakarotEthApi) will be moved to eth-rpc to reduce redundancy. Therefore, error conversion logic should also be updated accordingly to greatly limit the .map_err hell this move will (and has) induced.

Helpers

This file seems highly out of date.

I recommend completely deleting this file.
Some utils found in there will be useful, e.g. raw_kakarot_calldata, split_u256_into_field_elements and decode_eth_call_return, they should be updated, cleaned and moved.

Waiter

It's unclear to me if this is still needed. If so, it's very unlucky, as I don't understand the added complexity of having a transaction waiter.

I recommend investigating on testnet/mainnet if awaiting transactions using tokio is not enough to "wait" on them. Note that in our Python helpers, we did have a wait_for_confirmation util that polled too. It's worth investigating how ether-rs and alloy-rs do it.

Mod

The heart of the Kakarot Client. Started a PR #645 (will probably close this PR and restart after #644 is merged).

I recommend greatly pruning the logic inside this module. Most of the functions in KakarotEthApi can be inlined in eth-rpc crate, if they are only used once. For those that are extensively reused, we may want to keep them there, i.e. nonce, transaction_count_by_block and such.

Testing strategy

I recommend moving away from file-based testing and moving to integration like testing using a test sequencer.