Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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 <[email protected]>

* update PR reviews fixed

* update PR reviews fixed

---------

Co-authored-by: apoorvsadana <[email protected]>
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 <[email protected]>

* update PR reviews fixed

* update PR reviews fixed

---------

Co-authored-by: apoorvsadana <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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