401 fix issues of connecting to stellar overlay network on testnet #414

b-yap · 2023-10-09T17:20:22Z

closes #401;
Coming from #409

General overview of the changes:

This is to support both StellarMessage::TxSet and StellarMessage::GeneralizedTxSet after sending a StellarMessage::GetTxSet.

For pallets that use the spacewalk-primitives lib, I removed the substrate-stellar-sdk dependency.
- To be usable across all pallets, the features offchain and all-types of substrate-stellar-sdk are used.
Validating stellar transactions will now require the new TransactionSetType.
- move some code from spacewalk + added GeneralizedTxSet methods for retrieving a TransactionSet for pendulum-chain/spacewalk#401 substrate-stellar-sdk#21 for reference.
Broke down the type.rs inside vault/oracle to multiple files contained in the new type dir.
- The TxSetMap change its value type from TransactionSet to TransactionSetType
  - Note:
    - Affects all unit tests for TxSetMap. They have been commented out, marked as todo
    - Removed old and inapplicable test files:
      - clients/vault/resources/test/tx_sets
      - clients/vault/resources/test/tx_sets_for_testing
The changes in oracle are to support the GeneralizedTransactionSet message and to use TransactionSetType when creating proofs.
Log errors in stellar-relay-lib
Updated the metadata-standalone.scale

How to begin the review:

pallet dir
- all affected cargo.tomls MUST HAVE spacewalk-primitives as a dependency.
  - these cargo.tomls DO NOT use substrate-stellar-sdk
- all affected pallets MUST REPLACE substrate_stellar_sdk to primitives::stellar
- all fn validate_stellar_transaction(...) MUST REPLACE TransactionSet datatype param to TransactionSetType
- all construct_from_raw_encoded_xdr() MUST REPLACE TransactionSet datatype generic to TransactionSetType
vault/oracle dir
- types dir
  - limited_fifo_map.rs - TxSetMap MUST REPLACE TransactionSet to TransactionSetType

storage/impls.rs - comment out all TxSetMap or TxSetFIleHandler tests
collector dir
- proof_builder.rs - Proof struct MUST REPLACE TransactionSet to TransactionSetType
- collector.rs
  - update fn add_txset() to support 2 types: TransactionSet, GeneralizedTransactionSet
  - the tests will now use new samples from vault/resources/samples , and disable any TxSetMap tests ( todo)
- agent.rs - should handle both StellarMessage::TxSet and StellarMessage::GeneralizedTxSet messages

stellar-relay-lib
- service.rs - functions called inside a tokio::spawn() of overlay_connection.rs should log the errors instead of propagating them:
  - fn receiving_service(...)
  - fn connection_handler(...)
- tests/mod.rs
  - update stellar_overlay_should_receive_tx_set test case to use the testnet rather than mainnet (overlay_infos(false);)
    - add test to check the hashes of both StellarMessage::GetTxSet and StellarMessage::GeneralizedTxSet are THE SAME

https://github.com/pendulum-chain/spacewalk/actions/runs/6459806136/job/17536291785?pr=414

ebma · 2023-10-10T11:44:08Z

clients/stellar-relay-lib/src/connection/xdr_converter.rs

@@ -37,6 +37,7 @@ pub(crate) fn from_authenticated_message(message: &AuthenticatedMessage) -> Resu
 	message_to_bytes(message)
 }

+// todo: move to substrate-stellar-sdk


Just the macro?

I'm fine if we address this in a later PR.
We should keep in mind though that there are quite some changes here that still need to be refined (eg. out-commented code)

ebma · 2023-10-10T11:57:53Z

clients/vault/src/oracle/collector/collector.rs

+}
+
+impl AddTxSet<TransactionSetType> for ScpMessageCollector {
+	fn add_txset(&self, tx_set: TransactionSetType) -> Result<(), String> {


Should we rename this function to add_txset_type instead? I was a little confused reading the functions having all of them share the name.

ebma

Overall really great changes @b-yap 👍. Thanks for cleaning up the dependencies in the pallets, and also nice how you implemented the traits on the TransactionSetType such that we need to add as little changes as possible.

clients/vault/src/oracle/types/limited_fifo_map.rs

ebma · 2023-10-10T13:12:10Z

pallets/stellar-relay/src/tests.rs

+		let TransactionSetType::TransactionSet(mut set) = tx_set else {
+			panic!("must be of type `TransactionSet`")
+		};


Do we need to make this assumption here? If for some reason we change the tx_set returned by create_valid_dummy_scp_envelopes to return a GeneralizedTransactionSet in the future, this would break. Can we support both cases maybe?

clients/stellar-relay-lib/src/connection/services.rs

ebma · 2023-10-10T16:42:05Z

I'd approve the changes overall. I'll submit my approval once the other changes are addressed. Besides that, there is one more warning shown for a function name.

b-yap · 2023-10-10T16:46:26Z

everything is green! :D :D :D

…509349, #414 (comment), #414 (comment), #414 (comment)

b-yap · 2023-10-11T07:10:11Z

i'm going to merge this already @ebma

* partial of 9B2-04 | Logic Should be Moved To an Separate Function - Refactoring; partial of STL-03 | Lack of Validation for `destination_address` on `send_payment_to_address()` * 257 FIx Informational Issues - Logic Should be Moved To an Separate Function - Refactoring (#388) * 9B2-04 | Logic Should be Moved To an Separate Function - Refactoring - partial * 9B2-04 | Logic Should be Moved To an Separate Function - Refactoring - for System's fn run_service() * 9B2-04 | Logic Should be Moved To an Separate Function - Refactoring - fix from previous commit * remove stray log * remove extra space * https://github.com/pendulum-chain/spacewalk/pull/388/files#r1312732850, #388 (comment) * 257 Fix informational issues part 3 (#390) * IML-01 | Same Behavior Defined For Different Conditions * LI7-05 | Mismatch in Variable Name and Pallet Name * 9B2-02 | Inconsistent Comments * PRF-01 | Unhandled Error * LIH-03 | Values Length Not Validated in `feed_values` Function * CLI-01 | Confusing Function Naming * CLI-03 | Incorrect Error Type Thrown partial * CLI-03 | Incorrect Error Type Thrown * #390 (comment), #390 (comment), #390 (comment), #390 (comment), #390 (comment), https://github.com/pendulum-chain/spacewalk/actions/runs/6048709746/job/16414585252?pr=390#step:12:38 * https://github.com/pendulum-chain/spacewalk/actions/runs/6069946336/job/16465120232?pr=390#step:11:1573 * fix rustfmt error: https://github.com/pendulum-chain/spacewalk/actions/runs/6072497871/job/16472578544?pr=390 * CLI-01 | Confusing Function Naming * #390 (comment), #390 (comment), #390 (comment), #390 (comment), #390 (comment), https://github.com/pendulum-chain/spacewalk/actions/runs/6048709746/job/16414585252?pr=390#step:12:38 * STL-02 | Code Duplication * SRC-01 | Unused Methods and Storage * 9B2-05 | Commented Out Code (partial) * LBC-03 | Inconsistent `match` Expression * LIY-03 | Reduce Using `unwrap()` and `expect()` in Production Codebase * TYL-01 | Confusing Variable Naming * put back the removed dependencies * https://github.com/pendulum-chain/spacewalk/actions/runs/6095148657/job/16538119439?pr=393#step:11:91 * https://github.com/pendulum-chain/spacewalk/actions/runs/6096401270/job/16541879632#step:11:93 * https://github.com/pendulum-chain/spacewalk/actions/runs/6096401270/job/16541879632#step:11:93 * https://github.com/pendulum-chain/spacewalk/actions/runs/6097368982/job/16544863469?pr=393#step:11:94 * https://github.com/pendulum-chain/spacewalk/actions/runs/6098205449/job/16547533143?pr=393#step:13:19 * #393 (comment), partly #393 (review) * #393 (comment) * #393 (comment), #393 (review) for vault * #393 (comment) * fix https://github.com/pendulum-chain/spacewalk/actions/runs/6263576242/job/17008273208?pr=393#step:12:894 * https://github.com/pendulum-chain/spacewalk/actions/runs/6272775696/job/17034964214?pr=393#step:12:434 * https://github.com/pendulum-chain/spacewalk/actions/runs/6275148901/job/17041998689?pr=393#step:12:587 * https://github.com/pendulum-chain/spacewalk/actions/runs/6276542346/job/17091576766#step:12:476 * #393 (comment) * #393 (comment) and fix rebase * 401 fix issues of connecting to stellar overlay network on testnet (#414) * a 3rd try of #409 * cargo fmt and fix warnings in https://github.com/pendulum-chain/spacewalk/actions/runs/6459806136/job/17536291785?pr=414 * update the `substrate-stellar-sdk` version * #414 (comment), #414 (comment), #414 (comment), #414 (comment) * LIL-01 Dead Code in execute_replace() LIT-03 Panic can happen between correlated storage modifications * LIL-01 Dead Code in execute_replace() add back the comment

* SYT-03 | Unnecessary Variable * STL-03 | Lack of Validation for `destination_address` on `send_payment_to_address()` * SRL-01 | Usage of Hard-coded Strings * PAL-03 | Usage of Magic Numbers * PAL-02 | Unnecessary `Result<...>` Return Type * CLI-02 | Typos * 9B2-03 | Unused Errors * code cleanup for PAL-03 | Usage of Magic Numbers * code cleanup for PAL-02 | Unnecessary Result<...> Return Type * partial of 9B2-04 | Logic Should be Moved To an Separate Function - Refactoring; partial of STL-03 | Lack of Validation for `destination_address` on `send_payment_to_address()` * code cleanup * #385 (comment), #385 (comment), #385 (comment), #385 (comment), #385 (comment), #385 (comment), #385 (comment), #385 (comment) https://github.com/pendulum-chain/spacewalk/actions/runs/5936258794/job/16098086016?pr=385#step:10:33 * #385 (comment) * Use vault vs user wallet in integration tests * fix Unable to get required collateral for amount issue * #385 (comment) * removing the "last_known_cursor" and let the CI run it * https://github.com/pendulum-chain/spacewalk/actions/runs/5972713032/job/16203688884#step:9:144 * Fix `test_replace_succeeds` test case * fix test case due to changed `HorizonSubmissionError` * 257 FIx Informational Issues - Logic Should be Moved To an Separate Function - Refactoring (#388) * 9B2-04 | Logic Should be Moved To an Separate Function - Refactoring - partial * 9B2-04 | Logic Should be Moved To an Separate Function - Refactoring - for System's fn run_service() * 9B2-04 | Logic Should be Moved To an Separate Function - Refactoring - fix from previous commit * remove stray log * remove extra space * https://github.com/pendulum-chain/spacewalk/pull/388/files#r1312732850, #388 (comment) * 257 Fix informational issues part 3 (#390) * IML-01 | Same Behavior Defined For Different Conditions * LI7-05 | Mismatch in Variable Name and Pallet Name * 9B2-02 | Inconsistent Comments * PRF-01 | Unhandled Error * LIH-03 | Values Length Not Validated in `feed_values` Function * CLI-01 | Confusing Function Naming * CLI-03 | Incorrect Error Type Thrown partial * CLI-03 | Incorrect Error Type Thrown * #390 (comment), #390 (comment), #390 (comment), #390 (comment), #390 (comment), https://github.com/pendulum-chain/spacewalk/actions/runs/6048709746/job/16414585252?pr=390#step:12:38 * https://github.com/pendulum-chain/spacewalk/actions/runs/6069946336/job/16465120232?pr=390#step:11:1573 * fix rustfmt error: https://github.com/pendulum-chain/spacewalk/actions/runs/6072497871/job/16472578544?pr=390 * from https://github.com/pendulum-chain/spacewalk/pull/392/files#diff-0f76594421d4493b3bba1c9ed7864cdab7148f19c5d3a18a9bcc9a174bb85aa8 * from https://github.com/pendulum-chain/spacewalk/pull/392/files#diff-0f76594421d4493b3bba1c9ed7864cdab7148f19c5d3a18a9bcc9a174bb85aa8 * 257 fix informational issues part 4 (#393) * partial of 9B2-04 | Logic Should be Moved To an Separate Function - Refactoring; partial of STL-03 | Lack of Validation for `destination_address` on `send_payment_to_address()` * 257 FIx Informational Issues - Logic Should be Moved To an Separate Function - Refactoring (#388) * 9B2-04 | Logic Should be Moved To an Separate Function - Refactoring - partial * 9B2-04 | Logic Should be Moved To an Separate Function - Refactoring - for System's fn run_service() * 9B2-04 | Logic Should be Moved To an Separate Function - Refactoring - fix from previous commit * remove stray log * remove extra space * https://github.com/pendulum-chain/spacewalk/pull/388/files#r1312732850, #388 (comment) * 257 Fix informational issues part 3 (#390) * IML-01 | Same Behavior Defined For Different Conditions * LI7-05 | Mismatch in Variable Name and Pallet Name * 9B2-02 | Inconsistent Comments * PRF-01 | Unhandled Error * LIH-03 | Values Length Not Validated in `feed_values` Function * CLI-01 | Confusing Function Naming * CLI-03 | Incorrect Error Type Thrown partial * CLI-03 | Incorrect Error Type Thrown * #390 (comment), #390 (comment), #390 (comment), #390 (comment), #390 (comment), https://github.com/pendulum-chain/spacewalk/actions/runs/6048709746/job/16414585252?pr=390#step:12:38 * https://github.com/pendulum-chain/spacewalk/actions/runs/6069946336/job/16465120232?pr=390#step:11:1573 * fix rustfmt error: https://github.com/pendulum-chain/spacewalk/actions/runs/6072497871/job/16472578544?pr=390 * CLI-01 | Confusing Function Naming * #390 (comment), #390 (comment), #390 (comment), #390 (comment), #390 (comment), https://github.com/pendulum-chain/spacewalk/actions/runs/6048709746/job/16414585252?pr=390#step:12:38 * STL-02 | Code Duplication * SRC-01 | Unused Methods and Storage * 9B2-05 | Commented Out Code (partial) * LBC-03 | Inconsistent `match` Expression * LIY-03 | Reduce Using `unwrap()` and `expect()` in Production Codebase * TYL-01 | Confusing Variable Naming * put back the removed dependencies * https://github.com/pendulum-chain/spacewalk/actions/runs/6095148657/job/16538119439?pr=393#step:11:91 * https://github.com/pendulum-chain/spacewalk/actions/runs/6096401270/job/16541879632#step:11:93 * https://github.com/pendulum-chain/spacewalk/actions/runs/6096401270/job/16541879632#step:11:93 * https://github.com/pendulum-chain/spacewalk/actions/runs/6097368982/job/16544863469?pr=393#step:11:94 * https://github.com/pendulum-chain/spacewalk/actions/runs/6098205449/job/16547533143?pr=393#step:13:19 * #393 (comment), partly #393 (review) * #393 (comment) * #393 (comment), #393 (review) for vault * #393 (comment) * fix https://github.com/pendulum-chain/spacewalk/actions/runs/6263576242/job/17008273208?pr=393#step:12:894 * https://github.com/pendulum-chain/spacewalk/actions/runs/6272775696/job/17034964214?pr=393#step:12:434 * https://github.com/pendulum-chain/spacewalk/actions/runs/6275148901/job/17041998689?pr=393#step:12:587 * https://github.com/pendulum-chain/spacewalk/actions/runs/6276542346/job/17091576766#step:12:476 * #393 (comment) * #393 (comment) and fix rebase * 401 fix issues of connecting to stellar overlay network on testnet (#414) * a 3rd try of #409 * cargo fmt and fix warnings in https://github.com/pendulum-chain/spacewalk/actions/runs/6459806136/job/17536291785?pr=414 * update the `substrate-stellar-sdk` version * #414 (comment), #414 (comment), #414 (comment), #414 (comment) * LIL-01 Dead Code in execute_replace() LIT-03 Panic can happen between correlated storage modifications * LIL-01 Dead Code in execute_replace() add back the comment * https://github.com/pendulum-chain/spacewalk/actions/runs/6482197215/job/17610217546?pr=385#step:12:560 * https://github.com/pendulum-chain/spacewalk/actions/runs/6491793772/job/17629635299?pr=385#step:11:58 * fix clippy issues * cargo fmt --all --------- Co-authored-by: Marcel Ebert <[email protected]>

a 3rd try of #409

70c4790

b-yap changed the base branch from main to 257-fix-informational-issues-4 October 9, 2023 17:21

b-yap mentioned this pull request Oct 9, 2023

401 fix issues of connecting to stellar overlay network on testnet #409

Closed

cargo fmt and fix warnings in

52c7977

https://github.com/pendulum-chain/spacewalk/actions/runs/6459806136/job/17536291785?pr=414

ebma reviewed Oct 10, 2023

View reviewed changes

update the substrate-stellar-sdk version

3b3b503

b-yap requested a review from a team October 10, 2023 16:39

b-yap marked this pull request as ready for review October 10, 2023 16:39

https://github.com/pendulum-chain/spacewalk/pull/414#discussion_r1352…

1a93289

…509349, #414 (comment), #414 (comment), #414 (comment)

b-yap merged commit 242b157 into 257-fix-informational-issues-4 Oct 11, 2023
2 checks passed

b-yap deleted the 401-fix-issues-of-connecting-to-stellar-overlay-network-on-testnet-3 branch October 11, 2023 07:10

This was referenced Oct 11, 2023

Fix Informational Issues #385

Merged

257 fix informational issues part 4 #393

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

401 fix issues of connecting to stellar overlay network on testnet #414

401 fix issues of connecting to stellar overlay network on testnet #414

b-yap commented Oct 9, 2023 •

edited

Loading

ebma Oct 10, 2023

ebma Oct 10, 2023

ebma Oct 10, 2023

b-yap Oct 10, 2023

ebma left a comment

ebma Oct 10, 2023

b-yap Oct 10, 2023

ebma commented Oct 10, 2023

b-yap commented Oct 10, 2023

b-yap commented Oct 11, 2023

401 fix issues of connecting to stellar overlay network on testnet #414

401 fix issues of connecting to stellar overlay network on testnet #414

Conversation

b-yap commented Oct 9, 2023 • edited Loading

General overview of the changes:

How to begin the review:

ebma Oct 10, 2023

Choose a reason for hiding this comment

ebma Oct 10, 2023

Choose a reason for hiding this comment

ebma Oct 10, 2023

Choose a reason for hiding this comment

b-yap Oct 10, 2023

Choose a reason for hiding this comment

ebma left a comment

Choose a reason for hiding this comment

ebma Oct 10, 2023

Choose a reason for hiding this comment

b-yap Oct 10, 2023

Choose a reason for hiding this comment

ebma commented Oct 10, 2023

b-yap commented Oct 10, 2023

b-yap commented Oct 11, 2023

b-yap commented Oct 9, 2023 •

edited

Loading