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

Refactor stellar-relay-lib and vault/oracle crate implementations #372

Open
1 of 2 tasks
ebma opened this issue Jul 13, 2023 · 2 comments
Open
1 of 2 tasks

Refactor stellar-relay-lib and vault/oracle crate implementations #372

ebma opened this issue Jul 13, 2023 · 2 comments
Labels
priority:low Do it some day type:chore Maintenance tasks

Comments

@ebma
Copy link
Member

ebma commented Jul 13, 2023

Context

Our Spacewalk vault clients need to be able to communicate with Stellar's overlay network in order to be able to receive consensus messages. For this, we created the stellar-relay-lib crate. This crate can be used to establish a connection to a Stellar validator node in the overlay network and listen to and send messages to it. On the other hand, we have the vault/oracle crate, which facilitates the implementation of the proof building, which is very specific to Spacewalk and should not live in the 'generalized' stellar-relay-lib crate.

However, it appears that the overall structure of the code and the interfaces they expose are not always intuitive to use as a developer so we should consider refactoring them.

Refactorings

stellar-relay-lib

The implementation of the stellar-relay-lib crate was based on the JavaScript implementation of the js-stellar-node-connector project. The main component that is exposed to interact with the overlay network is the StellarOverlayConnection struct. The exposed interface/methods of the StellarOverlayConnection look good but the internal implementation could be improved. For example, the connect method is confusing because it creates some channels and spawns two services where it's not immediately clear why these are necessary. Also, some namings could be improved, e.g. what's a Connector, etc.

I'd propose we:

  • Try to simplify the architecture of the StellarOverlayConnection. Try to simplify/remove/reduce the usage of tokio channels. Maybe we can't get rid of them entirely but we can at least rewrite the code to be easier to read/understand
  • Think about improving namings of existing structs

The rest of the architecture can stay as is.

vault/oracle

This crate implements a so-called OracleAgent which is used by the vault client to build necessary consensus proofs for the stellar-relay pallet. It should be as easy to use as possible, ie. the interface should only expose the functions that are required by the vault client. Taking a step back from the current implementation and only thinking about what we'd wish for from the vault client side.

The OracleAgent should:

  • do something (to be defined).
@ebma ebma added the priority:low Do it some day label Jul 13, 2023
@prayagd
Copy link
Collaborator

prayagd commented Dec 7, 2023

@ebma Moving this to icebox assuming low priority

@b-yap
Copy link
Contributor

b-yap commented Dec 7, 2023

the 1st point is covered in #454.
StellarOverlayConnection now has 2 fields: a sender and receiver
Connector has cut 3 fields (retries, action_sender, relay_message_sender) and added 1: the write half of the stream. Owning the write half of the stream is beneficial since dropping the TcpStream only requires shutting down the write half.

The fn connect() now only spawns ONE thread; still contains 2 channels which is inevitable, since we need:

  1. a channel to communicate back and forth with the user/agent;
  2. a channel to communicate back and forth with the Stellar Node

Why is it necessary:

  • We are continuously reading messages from the stream, hence it has to exist in a separate thread. That already accounts to a need of a channel, since these messages has to be sent to the user/agent.
  • The user/agent needs to send GetTxSet and GetScpState messages to the node. I do not think it is wise to expose the write half of the stream outside of the lib, that is why we need another channel to relay these messages to the node.

@TorstenStueber TorstenStueber added the type:chore Maintenance tasks label Dec 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:low Do it some day type:chore Maintenance tasks
Projects
None yet
Development

No branches or pull requests

4 participants