Repository navigation
test: add faucet E2E test - #317
Conversation
174a605 to
b939900
Compare
| const DEFAULT_FAUCET_URL: &str = "https://faucet-api.testnet.miden.io"; | ||
| const REQUEST_TIMEOUT_MS: u64 = 30_000; | ||
| const MAX_SYNC_RETRIES: u32 = 3; | ||
| const MAX_SYNC_RETRIES: u32 = 10; |
There was a problem hiding this comment.
I increased this because 3 attempts was not enough when running the E2E test in the CI
There was a problem hiding this comment.
Could we implement this test in Rust instead? Also, I think the other scripts used for setup could just be Rust code as well. For the node, we might look at how the integration tests in the client spin it up, though using docker compose for it could be fine.
There was a problem hiding this comment.
I think keeping the script to spin the node is fine, the client needs to compile it to be able to pin a specific commit but that is not useful here.
Migrating the test itself to Rust would allow us to remove the install-miden-client.sh script.
There was a problem hiding this comment.
I migrated the test to Rust here 8c6c8b1
c4761f4 to
88eb679
Compare
88eb679 to
8c6c8b1
Compare
| # is 0.17.0-rc.3. The release run for that tag failed before it built anything, so no images | ||
| # were published for it and the newest ones are 0.17.0-rc.2. Remove this once the images are | ||
| # published. | ||
| NODE_VERSION: 0.17.0-rc.2 |
There was a problem hiding this comment.
The node docker images for 0.17.0-rc.3 haven't been published, so I pinned to the version to the previous one as a temporary fix.
There was a problem hiding this comment.
The node rc.4 has been released and it's docker images should also be available.
There was a problem hiding this comment.
I unpinned the node version in the CI test here 70c7f6a
TomasArrachea
left a comment
There was a problem hiding this comment.
Looks good. Left some suggestions for simplifying the code in the test.
| ClientBuilder::new() | ||
| .rpc(Arc::new(VerifyingRpcClient::new(GrpcClient::new( | ||
| &endpoint, | ||
| REQUEST_TIMEOUT_MS, | ||
| )))) | ||
| .store(Arc::new(store)) | ||
| .authenticator(Arc::new(keystore)) | ||
| .build() | ||
| .await | ||
| .expect("the client should connect to the node") |
There was a problem hiding this comment.
We should we able to use the convenience constructors:
| ClientBuilder::new() | |
| .rpc(Arc::new(VerifyingRpcClient::new(GrpcClient::new( | |
| &endpoint, | |
| REQUEST_TIMEOUT_MS, | |
| )))) | |
| .store(Arc::new(store)) | |
| .authenticator(Arc::new(keystore)) | |
| .build() | |
| .await | |
| .expect("the client should connect to the node") | |
| ClientBuilder::<FilesystemKeyStore>::new() | |
| .grpc_client(...) | |
| .filesystem_keystore(...)? | |
| .sqlite_store(...) | |
| .build() | |
| .await | |
| .expect("the client should connect to the node"); |
| let work_dir = std::path::Path::new(WORK_DIR); | ||
| // A fresh store every run, so a note or account from an earlier run cannot satisfy the test. | ||
| let _ = std::fs::remove_dir_all(work_dir); | ||
| std::fs::create_dir_all(work_dir.join("keys")).expect("the work directory should be created"); |
There was a problem hiding this comment.
We can use a temp_dir here. For example for the keystore: temp_dir().join(format!("keystore-{}", Uuid::new_v4()));. Same for the store.
| let key = AuthSecretKey::new_falcon512_poseidon2_with_rng(client.rng()); | ||
| let auth = AuthSingleSig::new(Approver::new( | ||
| key.public_key().to_commitment(), | ||
| AuthSchemeId::Falcon512Poseidon2, | ||
| )); | ||
|
|
||
| let account = AccountBuilder::new(rand::random()) | ||
| .account_type(AccountType::Public) | ||
| .with_component(auth) | ||
| .with_component(BasicWallet) | ||
| .build_with_schema_commitment() | ||
| .expect("the account should build"); |
There was a problem hiding this comment.
We can simplify this by using AuthSingleSig::from_public_key() or replace the builder with create_basic_wallet().
| for attempt in 1..=NOTE_ATTEMPTS { | ||
| client.sync_state().await.expect("the client should sync"); | ||
|
|
||
| let consumable = client | ||
| .get_consumable_notes(Some(account_id)) | ||
| .await | ||
| .expect("the consumable notes should be readable"); | ||
|
|
||
| let found = consumable.into_iter().find(|(record, _)| record.id() == Some(note_id)); | ||
| if let Some((record, _)) = found { | ||
| return record.try_into().expect("a committed note record holds its note"); | ||
| } | ||
|
|
||
| println!("Note not committed yet, attempt {attempt}/{NOTE_ATTEMPTS}"); | ||
| tokio::time::sleep(NOTE_DELAY).await; | ||
| } |
There was a problem hiding this comment.
I'm not sure but we might reuse the wait_for_consumable_notes() method if we use the miden_client::testing::TestClient.
| let request = TransactionRequestBuilder::new() | ||
| .input_notes(vec![(note, None)]) | ||
| .build() | ||
| .expect("the consume request should build"); | ||
|
|
||
| client | ||
| .submit_new_transaction(account_id, request) | ||
| .await | ||
| .expect("the node should accept the consume transaction"); |
There was a problem hiding this comment.
If we use the TestClient we can use TestClient::consume_notes for this, followed by wait_for_tx.
| fn fungible_balance(account: &Account) -> u64 { | ||
| account | ||
| .vault() | ||
| .assets() | ||
| .filter_map(|asset| asset.as_fungible().map(|asset| asset.amount().as_u64())) | ||
| .sum() | ||
| } |
There was a problem hiding this comment.
Use the client.account_reader(account_id).get_balance(faucet_id) for this.
|
@TomasArrachea thanks! addressed the comments here f3f9b32 |
| #[ignore = "needs a node, funding service and faucet, run it with `make test-e2e`"] | ||
| async fn request_tokens_and_consume_the_note() { | ||
| let faucet_url = env_or("FAUCET_URL", DEFAULT_FAUCET_URL); | ||
| let node_url = env_or("NODE_URL", DEFAULT_NODE_URL); |
There was a problem hiding this comment.
nit: I think we can use miden_client::rpc::Endpoint::localhost() for the node default
| tokio = { features = ["macros", "net", "rt-multi-thread", "time"], workspace = true } | ||
| axum = { features = ["tokio"], version = "0.8" } | ||
| miden-client = { features = ["testing"], workspace = true } | ||
| miden-client-sqlite-store = { workspace = true } |
There was a problem hiding this comment.
I think this dep is unused.
There was a problem hiding this comment.
Actually, this is for creating the client in the test itself
faucet/bin/faucet-client/tests/e2e.rs
Line 15 in f3f9b32
There was a problem hiding this comment.
Should we remove this script as well and start the faucet process in the Rust test itself? For the network script it might be okay to use the script for the docker containers.
There was a problem hiding this comment.
Removed the script and changed the Rust test to start the faucet here 8f4a492
Adds an E2E test that tests the faucet along with a local network and a funding service.