Skip to content
This repository was archived by the owner on May 24, 2025. It is now read-only.

small changes #89

Merged
merged 5 commits into from
Aug 20, 2024
Merged

small changes #89

merged 5 commits into from
Aug 20, 2024

Conversation

apoorvsadana
Copy link
Contributor

No description provided.

heemankv
heemankv reviewed Aug 19, 2024
View reviewed changes
crates/settlement-clients/ethereum/src/tests/mod.rs
let ethereum_settlement_client = EthereumSettlementClient::with_test_settings(
setup.provider.clone(),
Some(*STARKNET_CORE_CONTRACT_ADDRESS),
setup.rpc_url,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can possibly reduce sending rpc_url from EthereumTestBuilder by deriving from the provider. WDTY ?

crates/settlement-clients/ethereum/src/tests/mod.rs
// generating program output and blob vector
let program_output = get_program_output(fork_block_no);
let blob_data_vec = get_blob_data(fork_block_no);
// keeping 9 elements because the code accesses 8th index as program output
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I liked how you realised that this transaction doesn't actually require real program_output and blob_data_vec and hence it only creates with basic values.

crates/settlement-clients/ethereum/src/tests/mod.rs
/// Tests if the method is able to do a transaction with same function selector on a dummy contract.
/// If we impersonate starknet operator then we loose out on testing for validity of signature in the transaction.
/// Starknet core contract has a modifier `onlyOperator` that restricts anyone but the operator to send transaction to `updateStateKzgDa` method
/// And hence to test the signature and transaction via a dummy contract that has same function selector as `updateStateKzgDa`.
/// and anvil is for testing on fork Eth.
async fn update_state_blob_with_dummy_contract_works(#[case] fork_block_no: u64) {
env::set_var("SHOULD_IMPERSONATE_ACCOUNT", "false");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how you have removed SHOULD_IMPERSONATE_ACCOUNT as an environment dependency. and are using the Builder pattern to impersonate account.

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

#[rstest]
#[tokio::test]
#[case::basic(20468828)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how now this test update_state_blob_with_dummy_contract_works is completely independent of the Ethereum block number.

crates/settlement-clients/ethereum/src/tests/mod.rs
dotenvy::from_filename(&*ENV_FILE_PATH).expect("Could not load .env.test file.");
struct EthereumTest {
_anvil: AnvilInstance,
provider: alloy::providers::RootProvider<alloy::transports::http::Http<reqwest::Client>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can import RootProvider and Client to make this look cleaner,

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

// Setup Anvil
let anvil = Anvil::new()
.port(*PORT)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how this completely removes the dependency of spawning Anvil on a particular PORT rather the Url Anvil spins upto is provided to provider and as rpc_url.

heemankv
heemankv reviewed Aug 20, 2024
View reviewed changes
crates/settlement-clients/ethereum/src/lib.rs Outdated
// Note : changing between "0" and "1" is handled automatically by each test function, `no` manual change in `env.test` is needed.
if let Some(impersonate_account) = impersonate_account {
let nonce =
provider.get_transaction_count(impersonate_account).await.unwrap().to_string().parse::<u64>().unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already send the nonce to the update_state_with_blobs method then why are we recalculating it here ?

.env.test
@@ -28,7 +28,6 @@ MONGODB_CONNECTION_STRING="mongodb://localhost:27017"


# Ethereum Settlement
DEFAULT_SETTLEMENT_CLIENT_RPC="http://localhost:3000"
Copy link
Contributor

@heemankv heemankv Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed in test cases outside of Settlement Client, hence keeping

@coveralls
Copy link

Coverage Status

coverage: 63.845% (+0.6%) from 63.286%
when pulling 422fd16 on settlement_changes
into 30e8584 on test/settlement-client.

@heemankv heemankv merged commit ef5a562 into test/settlement-client Aug 20, 2024
8 checks passed
apoorvsadana added a commit that referenced this pull request Aug 23, 2024
@heemankv @apoorvsadana
Test: Ethereum Settlement Client (#81)
11dc132 
* 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 <heemankv@gmail.com>

* update PR reviews fixed

* update PR reviews fixed

---------

Co-authored-by: apoorvsadana <95699312+apoorvsadana@users.noreply.github.com>
ocdbytes pushed a commit that referenced this pull request Oct 16, 2024
@heemankv @apoorvsadana
Test: Ethereum Settlement Client (#81)
334b3cc 
* 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 <heemankv@gmail.com>

* update PR reviews fixed

* update PR reviews fixed

---------

Co-authored-by: apoorvsadana <95699312+apoorvsadana@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@heemankv heemankv heemankv left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@apoorvsadana @coveralls @heemankv