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.

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

482 add stellar testnet archive urls to vault client config #524

Merged

Conversation

gianfra-t
Copy link
Contributor

@gianfra-t gianfra-t commented May 8, 2024

Regarding #482

About

Adds archive url's for Stellar testent, and code changes such that we don't discriminate anymore between testnet and mainnet for fetching this values.

Notable changes

  • Previously we used this condition to skip fetching envelopes from an archive node if the vault was running on testnet. Now the behavior is the same for both testnet and public.
  • Transforming the slot index to hex to form the archive url had to be slightly modified such that a full 8 bytes representation of the slot index is formed, padding the most significant bytes with 0 if necessary. This was not necessary before since mainnet indexes are currently large enough that they use 8 bytes without padding, which is not the case for testnet. (See implementation here).
  • Adds two tests that mirror the public counterpart to test fetching and opening transaction sets and envelopes.
  • Adds the archive url's to the configuration files.

@gianfra-t gianfra-t linked an issue May 8, 2024 that may be closed by this pull request
@gianfra-t gianfra-t marked this pull request as ready for review May 8, 2024 19:30
@gianfra-t
Copy link
Contributor Author

@pendulum-chain/devs beyond the simple unit tests added, does anyone know if there is a configuration by which we can trigger in the integration tests a situation in which the client will try to look for these archives?

@gianfra-t gianfra-t requested a review from a team May 8, 2024 19:46
@b-yap
Copy link
Contributor

b-yap commented May 9, 2024

@gianfra-t Waiting time is 5 minutes:

During normal operation, a validating stellar-core server will save a "checkpoint" of its recent operations to XDR files, compress them, and publish them to all of its configured writable history archives once every 64 ledgers (about once every 5 minutes).

There is an integration test that Marcel added:

async fn test_issue_execution_succeeds_from_archive() {
let is_public_network = true;
test_with_vault(
is_public_network,
|client, _vault_wallet, user_wallet, _oracle_agent, vault_id, vault_provider| async move {
let user_provider = setup_provider(client.clone(), AccountKeyring::Dave).await;
let public_key = default_vault_stellar_address_as_binary(is_public_network);
let issue_amount = upscaled_compatible_amount(100);
let vault_collateral = get_required_vault_collateral_for_issue(
&vault_provider,
issue_amount,
vault_id.wrapped_currency(),
vault_id.collateral_currency(),
)
.await;
assert_ok!(
vault_provider
.register_vault_with_public_key(&vault_id, vault_collateral, public_key)
.await
);
// This call returns a RequestIssueEvent, not the IssueRequest from primitives
let issue = user_provider
.request_issue(issue_amount, &vault_id)
.await
.expect("Requesting issue failed");
// Send a payment to the destination of the issue request (ie the targeted vault's
// stellar account)
let stroop_amount = primitives::BalanceConversion::lookup(issue.amount + issue.fee)
.expect("Conversion should not fail");
let destination_public_key = PublicKey::from_binary(issue.vault_stellar_public_key);
let stellar_asset =
primitives::AssetConversion::lookup(issue.asset).expect("Asset not found");
let transaction_response = send_payment_to_address(
user_wallet,
destination_public_key,
stellar_asset,
stroop_amount.try_into().unwrap(),
issue.issue_id.0,
false,
)
.await
.expect("Sending payment failed");
assert!(transaction_response.successful);
let slot = transaction_response.ledger as u64;
// We sleep here in order to wait for the fallback to the archive to be necessary
sleep(Duration::from_secs(5 * 60)).await;
let shutdown_tx = ShutdownSender::new();
let stellar_config = random_stellar_relay_config(is_public_network);
let vault_stellar_secret = get_source_secret_key_from_env(is_public_network);
// Create new oracle agent with the same configuration as the previous one
let oracle_agent =
start_oracle_agent(stellar_config.clone(), &vault_stellar_secret, shutdown_tx)
.await
.expect("failed to start agent");
let oracle_agent = Arc::new(oracle_agent);
// Loop pending proofs until it is ready
let proof = oracle_agent.get_proof(slot).await.expect("Proof should be available");
let tx_envelope_xdr_encoded = transaction_response.envelope_xdr;
let (envelopes_xdr_encoded, tx_set_xdr_encoded) = proof.encode();
join(
assert_event::<EndowedEvent, _>(TIMEOUT, user_provider.clone(), |x| {
if &x.who == user_provider.get_account_id() {
assert_eq!(x.amount, issue.amount - issue.fee);
true
} else {
false
}
}),
user_provider
.execute_issue(
issue.issue_id,
&tx_envelope_xdr_encoded,
envelopes_xdr_encoded.as_bytes(),
tx_set_xdr_encoded.as_bytes(),
)
.map(Result::unwrap),
)
.await;
},
)
.await;
}

You can see in line 663:

sleep(Duration::from_secs(5 * 60)).await;




@ebma if the archives are already available in testnet, should we revert the:
Screenshot 2024-05-09 at 5 09 46 PM
testchain to only have 1?
It could reduce the code.

@gianfra-t
Copy link
Contributor Author

Thanks for pointing that out @b-yap ! I replaced here such that the testnet would be used and it works (locally).

I didn't add a new test_issue_execution_succeeds_from_archive just for testnet since I think it may be redundant, but we could.

@b-yap b-yap changed the base branch from main to test-pallet_asset_tx_payment May 10, 2024 13:41
@b-yap b-yap changed the base branch from test-pallet_asset_tx_payment to main May 10, 2024 13:42
@b-yap b-yap force-pushed the 482-add-stellar-testnet-archive-urls-to-vault-client-config branch from 7c87dd8 to 320c1c3 Compare May 10, 2024 13:48
@b-yap
Copy link
Contributor

b-yap commented May 10, 2024

hhhm, something wrong with the rustfmt; it's reverted back. I tried to do cargo +nightly-2024-02-09 and it accepts ; after returns.

@gianfra-t
Copy link
Contributor Author

I did run fmt because the CI failed on the format check, do you think this broke something?

@b-yap
Copy link
Contributor

b-yap commented May 13, 2024

trailing_semicolon is an unstable option, and so are the others. Probably makes sense why it works only now and then.

Torsten commented on this before. I didn't want the reformatting to overlap with this PR.

Apologies on the unverified commits. 😭 @gianfra-t if you can, please sign the commits again 🙇.

@gianfra-t
Copy link
Contributor Author

Oh okay, sorry! So we have to be careful and leave the format to the pre-commit hook @b-yap? Signing the commits again no problem.

gianfra-t and others added 7 commits May 13, 2024 08:43
…om testnet archive instead of skipping

Signed-off-by: Gianfranco <g.tasteri@gmail.com>
Signed-off-by: Gianfranco <g.tasteri@gmail.com>
Signed-off-by: Gianfranco <g.tasteri@gmail.com>
Signed-off-by: Gianfranco <g.tasteri@gmail.com>
This reverts commit 320c1c3.
@gianfra-t gianfra-t force-pushed the 482-add-stellar-testnet-archive-urls-to-vault-client-config branch from 62c8917 to 189765c Compare May 13, 2024 11:47
@ebma
Copy link
Member

ebma commented May 13, 2024

@gianfra-t and @b-yap I created a new issue #525 for the formatting. I already noticed this problem last week and it's quite annoying that we now have this mismatch between the formatting of the precommit hook vs what the CI expects.

if the archives are already available in testnet, should we revert the testchain to only have 1? It could reduce the code.

@b-yap I don't think we should. The main reason I created the tests is to be able to replicate issues that only seem to occur on Stellar mainnet. Being able to test on both Stellar test and main network is quite valuable IMO.

I didn't add a new test_issue_execution_succeeds_from_archive just for testnet since I think it may be redundant, but we could.

True, it's redundant but I'd say we add the test case anyway. We can afford to have the CI run 3 extra minutes in order to be safe that restoring messages from archives works as expected. In the future it could happen that the SDF introduces some changes that break our logic and as they will introduce them on testnet first, we would be able to catch it earlier.

@@ -10,5 +10,7 @@
"version_str": "stellar-core 20.2.0.rc3 (34d82fc00426643e16b7ad59c9fde169b778eb4b)",
"is_pub_net": false
},
"stellar_history_archive_urls": []
"stellar_history_archive_urls": [
"http://history.stellar.org/prd/core-testnet/core_testnet_001"
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason why we don't include core_testnet_002 and core_testnet_003?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No particular reason, in fact I thought about adding all of them, but then looking at the analogous stellar_relay_config_mainnet_iowa.json I see that it only uses one of the mainnet archives.

You think every config file should have all the possible archive url's @b-yap, including for mainnet?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see. I was wondering whether you paired them on purpose: like sdftest1 for core_testnet_001; sdftest2 for core_testnet_002, sdftest3 for core_testnet_003.

In that case having only 1 url in the list is a-ok 👌

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess the only argument in favor of separating them could be to have a more fine-grained control in case one of them is not working, so that the selection is not random when trying to fetch.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is just some legacy fragment, because in the very beginning we only knew about one Stellar archive URL. By now, we know 'all' of the official ones and there is no good reason not to add all of them to the array.

It might be least confusing if we streamline this across all the configuration files and make each of them list all archives (for the respective network only, of course).

Copy link
Contributor Author

@gianfra-t gianfra-t May 14, 2024

Choose a reason for hiding this comment

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

Alright, also makes sense. Added here.
We could also add stellar-history.. ones as well as core-live but seems to me a bit confusing.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm.. let's maybe still have all 6 in the mainnet configurations to signify that all of them can be used.

Copy link
Member

@ebma ebma left a comment

Choose a reason for hiding this comment

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

Looks good to me too 👍

clients/vault/tests/vault_integration_tests.rs Outdated Show resolved Hide resolved
@@ -10,5 +10,7 @@
"version_str": "stellar-core 20.2.0.rc3 (34d82fc00426643e16b7ad59c9fde169b778eb4b)",
"is_pub_net": false
},
"stellar_history_archive_urls": []
"stellar_history_archive_urls": [
"http://history.stellar.org/prd/core-testnet/core_testnet_001"
Copy link
Member

Choose a reason for hiding this comment

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

I think this is just some legacy fragment, because in the very beginning we only knew about one Stellar archive URL. By now, we know 'all' of the official ones and there is no good reason not to add all of them to the array.

It might be least confusing if we streamline this across all the configuration files and make each of them list all archives (for the respective network only, of course).

@b-yap b-yap self-requested a review May 16, 2024 08:46
@gianfra-t gianfra-t merged commit badbbfc into main May 16, 2024
2 checks passed
@gianfra-t gianfra-t deleted the 482-add-stellar-testnet-archive-urls-to-vault-client-config branch May 16, 2024 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Stellar testnet archive URLs to vault client config
3 participants