Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Roles Technical Debt #1162

Closed
jbesraa opened this issue Sep 8, 2024 · 1 comment
Closed

Roles Technical Debt #1162

jbesraa opened this issue Sep 8, 2024 · 1 comment

Comments

@jbesraa
Copy link
Contributor

jbesraa commented Sep 8, 2024

While working on #1066 and the integration tests for the roles, I noticed a few technical debts that would need to be addressed in a future refactor:

** Note that the goal of this issue is to document the different problems within the roles code, but we are not planning to resolve them until we are done with #845.

The following points are related almost always to all of the roles:

  1. Problem: async-channel is used within tokio context which can result in an unexpected bugs
    Solution: use tokio channels https://tokio.rs/tokio/tutorial/channels

  2. Problem: Some struct(s) function(s) have weird signatures like self_: &Arc<Mutex<Self>>
    Solution: Wrap only the properties that need to be mutated in a Mutex

  3. Problem: An internal Mutex implementation is used
    Solution: use https://docs.rs/tokio/latest/tokio/sync/struct.Mutex.html instead

  4. Problem: Duplicate code between the different roles. Some roles need Downstream service, others need Upstream. Usually this is just a server.
    Solution: use a common definition for Upstream, Downstream across the roles where possible

  5. Problem: Config properties are not clear and not grouped in structs
    Solution: Group properties into structs with more clear names

  6. Problem: Roles have more responsibilities than needed. For example the pool role is required to connect to the TemplateProvider instead of receiving the TemplateProvider connection as an input.
    Solution: Minimize code inside role::start functions across the different roles to only do whats that role is responsible for.

@Fi3
Copy link
Collaborator

Fi3 commented Sep 9, 2024

  1. we should remove async-channel cause most of the time is not needed or we can slightly change the implementation, that would result in a dependency removed that is good. Said that I want to add that I'm pretty sure that using them withing tokio context is ok.
  2. This make sense, but sometimes is faster to have one single Mutex than a lot of small ones, so we should go case by case here.
  3. We use the internal Mutex cause it have safe_lock that IMO is an interface fair superior than lock, it make a lot of foot guns harder or not possible (lock between await, not releasing the lock ecc ecc). Also keep in mind that tokio Mutex is not the right choice, most of the time is better to use std Mutex, you can find more about it on the tokio documentation.

@stratum-mining stratum-mining locked and limited conversation to collaborators Sep 16, 2024
@plebhash plebhash converted this issue into discussion #1166 Sep 16, 2024

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants