Test: Ethereum Settlement Client #81

heemankv · 2024-08-12T09:30:00Z

This PR adds tests for `Ethereum Settlement Client`.

External Dependencies :

Anvil for forking and running local test chain.
Ethereum RPC.

Points to note :

Test code blocks are included with #[cfg(test)] between prod code, to ensure possibility of testing, please feel free to suggest any other way.
Two tests : update_state_blob_with_dummy_contract_works & update_state_blob_with_impersonation_works use a test param to make aware about impersonation or dummy contract call.
Nonce: To ensure consistency in nonce while doing multiple update state transactions we extract the nonce fetch call from the child : update_state_with_blobs and manage nonce from process_job, this doesn't affect the distributed fashion of code since each machine will have a pre-existing lock over the jobs.
( update_state_with_blobs is called by update_state_for_block which is called inside a loop by process_job)

…test

crates/settlement-clients/ethereum/src/lib.rs

crates/settlement-clients/ethereum/src/test_data/ABI/starknet_core_contract.json

coveralls · 2024-08-16T11:53:27Z

coverage: 65.198% (+7.0%) from 58.192%
when pulling f9d3682 on test/settlement-client
into 5728df2 on main.

…ks & update_state_blob_with_dummy_contract_works

crates/settlement-clients/ethereum/src/lib.rs

crates/settlement-clients/ethereum/src/conversion.rs

Reworking Settlement Client test cases to be independent of env vars and work in minimalism. --------- Co-authored-by: Heemank Verma <[email protected]>

odesenfans

I would need to understand the context a bit more to review this PR meaningfully. I added a few comments but they're not super interesting.

odesenfans · 2024-08-21T13:01:20Z

.env.test

@@ -15,7 +15,7 @@ MADARA_RPC_URL="http://localhost:3000"
 ETHEREUM_RPC_URL="http://localhost:3001"
 MEMORY_PAGES_CONTRACT_ADDRESS="0x000000000000000000000000000000000001dead"
 PRIVATE_KEY="0xdead"
-ETHEREUM_PRIVATE_KEY="0x000000000000000000000000000000000000000000000000000000000000beef"
+ETHEREUM_PRIVATE_KEY="0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80"


Are you okay with this being public? If this is not a problem, document why this is set in Github.

Ideally, we can remove this but it's genuinely no harm since this is Private_key provided by Anvil, will add a comment to explain the same

odesenfans · 2024-08-21T13:35:10Z

crates/settlement-clients/ethereum/Cargo.toml

@@ -4,11 +4,14 @@ version.workspace = true
 edition.workspace = true

 [dependencies]
-alloy = { workspace = true, features = ["full"] }
+alloy = { workspace = true, features = ["full", "node-bindings"] }
+alloy-primitives = { version = "0.7.7", default-features = false }


Unrelated to the PR, but I think it makes sense to define all your dependencies at the workspace level and then pull them in each crate like you're doing with alloy.

Created an issue for the same to be thoroughly checked later on : #99

odesenfans · 2024-08-21T14:00:04Z

crates/settlement-clients/ethereum/src/conversion.rs

+use alloy_primitives::FixedBytes;
+use c_kzg::{Blob, KzgCommitment, KzgProof, KzgSettings};
+use color_eyre::{eyre::ContextCompat, Result as EyreResult};
+use std::fmt::Write;

 /// Converts a `&[Vec<u8>]` to `Vec<U256>`. Each inner slice is expected to be exactly 32 bytes long.


This comment is slightly wrong as you're taking &[[u8; 32]]. Update it?

Valid, updated.

odesenfans · 2024-08-21T14:06:29Z

crates/settlement-clients/ethereum/src/conversion.rs

+
+// Function to convert a slice of u8 to a padded hex string
+// Function only takes a slice of length up to 32 elements
+// Pads the value on the right side with zeros only if the converted string has lesser than 64 characters.


Use docstrings to document functions (/// instead of //).

Valid, updated.

odesenfans · 2024-08-21T14:08:43Z

crates/settlement-clients/ethereum/src/conversion.rs

+    Ok(input_data)
+}
+
+pub(crate) fn vec_u8_32_to_hex_string(data: Vec<[u8; 32]>) -> String {


Why do you need ownership here? Can't you pass a slice in all cases?

Valid, improved. using &[[u8; 32]] now

odesenfans · 2024-08-21T14:10:36Z

crates/settlement-clients/ethereum/src/tests/mod.rs

+}
+
+struct EthereumTest {
+    _anvil: AnvilInstance,


Why are you prefixing this with an underscore?

So,
We notices the an anvil server is needed to be in scope in any function that is using RPC calls to anvil.

We create anvil server in EthereumTestBuilder if we don't pass it to update_state_blob_with_dummy_contract_works we get ConnectionRefused Error suggesting that the server that was started in EthereumTestBuilder has now been closed.

I believe this has to do with how rust handles variable scoping.

So, _anvil as a variable is never used, it's only there to be in the scope, hence the _.

This is my understanding, please feel free to correct it / broaden the perspective / suggest an alternative & optimised way to achieve the same.

* update: settlement-client: ethereum test cases for conversion fns * update: removed .expect from slice_u8_to_u256 * update: Eth Settlement client: tests for conversions * update: Eth Settlement client : prepare_sidecar * update: settlement_client: eth: prepare_sidecar tests * chore: optimisations * update: working test case * update: working test #2 * update: added cfg test for update_state_with_blobs * update: cleaner cfg(test) implemented code for update_state_and_blob_test * chore: liniting fixes * update: linting fixes * docs: changelog * update: Nonce prefetch for state_update * update: creation of input_data works * update: using correct input bytes * chore: lint fix * update: test normal transaction * update: dummy contract and impersonation tests ready * update: test cases for settlement client * chore: lint fixes * chore: fix lints * update: Changes for PR review * chore: fixing test cases for eth settlement client * update: tests fix * chore: lint fix * chore: lint fix * update: path fix * update: testing anvil install on gh * update: fixing path * update: added Blast rpc for eth * update: Coverage CI fixes * update: correct Pr checks * update: PR reviews fixes * update: PR reviews fixes * update: adding rationale for update_state_blob_with_impersonation_works & update_state_blob_with_dummy_contract_works * update: cleaner test integration on update_state_with_blobs * update: removing EthProvider * Reworking Settlement Client Changes (#89) Reworking Settlement Client test cases to be independent of env vars and work in minimalism. --------- Co-authored-by: Heemank Verma <[email protected]> * update PR reviews fixed * update PR reviews fixed --------- Co-authored-by: apoorvsadana <[email protected]>

heemankv requested a review from apoorvsadana August 12, 2024 09:30

heemankv self-assigned this Aug 12, 2024

heemankv force-pushed the test/settlement-client branch from e35fc0b to 0ea20a1 Compare August 12, 2024 10:08

heemankv changed the title ~~Test: Settlement Client~~ [inProgress] Test: Settlement Client Aug 13, 2024

heemankv and others added 20 commits August 14, 2024 16:10

update: settlement-client: ethereum test cases for conversion fns

84a3819

update: removed .expect from slice_u8_to_u256

0c6d868

update: Eth Settlement client: tests for conversions

f77093b

update: Eth Settlement client : prepare_sidecar

c23a4a6

update: settlement_client: eth: prepare_sidecar tests

5e2ea5e

chore: optimisations

f435b1d

update: working test case

229e50d

update: working test #2

aa1bef2

update: added cfg test for update_state_with_blobs

3158c48

update: cleaner cfg(test) implemented code for update_state_and_blob_…

db1a675

…test

chore: liniting fixes

d562e4c

update: linting fixes

69cc0b7

docs: changelog

46fc944

update: Nonce prefetch for state_update

c6bfdce

update: creation of input_data works

f3fc826

update: using correct input bytes

d28b98b

chore: lint fix

a03f9db

update: test normal transaction

06d97cd

update: dummy contract and impersonation tests ready

5aa7b10

update: test cases for settlement client

e7023af

heemankv force-pushed the test/settlement-client branch from 1edc905 to e7023af Compare August 14, 2024 10:41

heemankv changed the title ~~[inProgress] Test: Settlement Client~~ Test: Settlement Client Aug 14, 2024

heemankv changed the title ~~Test: Settlement Client~~ Test: Ethereum Settlement Client Aug 14, 2024

heemankv added the tests label Aug 14, 2024

chore: lint fixes

391d693

heemankv force-pushed the test/settlement-client branch from 9b8c738 to 391d693 Compare August 14, 2024 15:44

heemankv added 4 commits August 16, 2024 13:54

update: path fix

3a125ad

update: testing anvil install on gh

e2a0bed

update: fixing path

92aa145

update: added Blast rpc for eth

6c03f7b

apoorvsadana reviewed Aug 16, 2024

View reviewed changes

crates/settlement-clients/ethereum/src/lib.rs Outdated Show resolved Hide resolved

crates/settlement-clients/ethereum/src/lib.rs Outdated Show resolved Hide resolved

crates/settlement-clients/ethereum/src/test_data/ABI/starknet_core_contract.json Outdated Show resolved Hide resolved

heemankv temporarily deployed to test August 16, 2024 20:42 — with GitHub Actions Inactive

heemankv force-pushed the test/settlement-client branch from 39317e5 to 6c03f7b Compare August 16, 2024 21:39

heemankv added 2 commits August 17, 2024 03:18

update: Coverage CI fixes

a3c0863

update: correct Pr checks

d0ddfde

heemankv force-pushed the test/settlement-client branch from e2dba9a to 0fa24ba Compare August 17, 2024 05:14

update: PR reviews fixes

4de6e8b

heemankv force-pushed the test/settlement-client branch from 0fa24ba to 4de6e8b Compare August 17, 2024 05:15

heemankv added 2 commits August 17, 2024 11:10

update: PR reviews fixes

ebe967e

update: adding rationale for update_state_blob_with_impersonation_wor…

ef35e52

…ks & update_state_blob_with_dummy_contract_works

apoorvsadana reviewed Aug 17, 2024

View reviewed changes

heemankv and others added 3 commits August 18, 2024 14:18

update: cleaner test integration on update_state_with_blobs

a8f4bb1

update: removing EthProvider

30e8584

Reworking Settlement Client Changes (#89)

ef5a562

Reworking Settlement Client test cases to be independent of env vars and work in minimalism. --------- Co-authored-by: Heemank Verma <[email protected]>

apoorvsadana approved these changes Aug 20, 2024

View reviewed changes

odesenfans reviewed Aug 21, 2024

View reviewed changes

heemankv mentioned this pull request Aug 23, 2024

Workspace oriented dependencies. #99

Closed

heemankv added 2 commits August 23, 2024 18:26

Merge branch 'main' into test/settlement-client

6b7a656

Merge branch 'main' into test/settlement-client

8e9c13d

odesenfans approved these changes Aug 23, 2024

View reviewed changes

heemankv added 2 commits August 23, 2024 19:23

update PR reviews fixed

b93372e

update PR reviews fixed

f9d3682

apoorvsadana merged commit 11dc132 into main Aug 23, 2024
9 checks passed

heemankv mentioned this pull request Sep 6, 2024

Evaluate conversion functions for all crates. #84

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test: Ethereum Settlement Client #81

Test: Ethereum Settlement Client #81

heemankv commented Aug 12, 2024 •

edited

Loading

coveralls commented Aug 16, 2024 •

edited

Loading

odesenfans left a comment

odesenfans Aug 21, 2024

heemankv Aug 23, 2024

odesenfans Aug 21, 2024

heemankv Aug 23, 2024

odesenfans Aug 21, 2024

heemankv Aug 23, 2024

odesenfans Aug 21, 2024

heemankv Aug 23, 2024

odesenfans Aug 21, 2024

heemankv Aug 23, 2024

odesenfans Aug 21, 2024

heemankv Aug 23, 2024

Test: Ethereum Settlement Client #81

Test: Ethereum Settlement Client #81

Conversation

heemankv commented Aug 12, 2024 • edited Loading

This PR adds tests for Ethereum Settlement Client.

External Dependencies :

Points to note :

coveralls commented Aug 16, 2024 • edited Loading

odesenfans 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

heemankv commented Aug 12, 2024 •

edited

Loading

This PR adds tests for `Ethereum Settlement Client`.

coveralls commented Aug 16, 2024 •

edited

Loading