-
Notifications
You must be signed in to change notification settings - Fork 135
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
Move TranslatorSv2
lib code out of main.rs
#1092
Move TranslatorSv2
lib code out of main.rs
#1092
Conversation
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
🚨 1 ALERT: Threshold Boundary Limit exceeded!
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
🚨 2 ALERTS: Threshold Boundary Limits exceeded!
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
825b8c9
to
8461b4b
Compare
TranslatorSv2
TranslatorSv2
lib code out of main.rs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK
debug!("Starting up status listener"); | ||
// Check all tasks if is_finished() is true, if so exit | ||
loop { | ||
let task_status = tokio::select! { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let task_status = tokio::select! { | |
let task_status = select! { |
Select macro can be added to above tokio import
pub struct TranslatorProxyUpstream { | ||
address: String, | ||
port: u16, | ||
authority_pubkey: Secp256k1PublicKey, | ||
difficulty_config: UpstreamDifficultyConfig, | ||
} | ||
|
||
pub struct TranslatorProxyDownstream { | ||
address: String, | ||
port: u16, | ||
difficulty_config: DownstreamDifficultyConfig, | ||
} | ||
|
||
impl ProxyConfig { | ||
pub fn new( | ||
upstream: TranslatorProxyUpstream, | ||
downstream: TranslatorProxyDownstream, | ||
max_supported_version: u16, | ||
min_supported_version: u16, | ||
min_extranonce2_size: u16, | ||
) -> Self { | ||
Self { | ||
upstream_address: upstream.address, | ||
upstream_port: upstream.port, | ||
upstream_authority_pubkey: upstream.authority_pubkey, | ||
downstream_address: downstream.address, | ||
downstream_port: downstream.port, | ||
max_supported_version, | ||
min_supported_version, | ||
min_extranonce2_size, | ||
downstream_difficulty_config: downstream.difficulty_config, | ||
upstream_difficulty_config: upstream.difficulty_config, | ||
} | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since these structs are open to anyone starting a translator and are used for the initial proxy setup, it’s important to make sure they're easy to use and less error-prone. These structs have a lot of fields and can get pretty lengthy, so I suggest using the Builder design pattern to simplify things. This approach will make initialization less prone to mistakes and more straightforward. Here’s a quick example:
#[derive(Debug, Deserialize, Clone)]
struct ProxyConfig {
pub upstream_address: String,
pub upstream_port: u16,
pub upstream_authority_pubkey: Secp256k1PublicKey,
pub downstream_address: String,
pub downstream_port: u16,
pub max_supported_version: u16,
pub min_supported_version: u16,
pub min_extranonce2_size: u16,
pub downstream_difficulty_config: DownstreamDifficultyConfig,
pub upstream_difficulty_config: UpstreamDifficultyConfig,
}
pub struct ProxyConfigBuilder {
upstream: Option<TranslatorProxyUpstream>,
downstream: Option<TranslatorProxyDownstream>,
max_supported_version: Option<u16>,
min_supported_version: Option<u16>,
min_extranonce2_size: Option<u16>,
}
impl ProxyConfigBuilder {
pub fn builder() -> Self {
Self {
upstream: None,
downstream: None,
max_supported_version: None,
min_supported_version: None,
min_extranonce2_size: None,
}
}
pub fn upstream(mut self, upstream: TranslatorProxyUpstream) -> Self {
self.upstream = Some(upstream);
self
}
pub fn downstream(mut self, downstream: TranslatorProxyDownstream) -> Self {
self.downstream = Some(downstream);
self
}
pub fn max_supported_version(mut self, version: u16) -> Self {
self.max_supported_version = Some(version);
self
}
pub fn min_supported_version(mut self, version: u16) -> Self {
self.min_supported_version = Some(version);
self
}
pub fn min_extranonce2_size(mut self, size: u16) -> Self {
self.min_extranonce2_size = Some(size);
self
}
pub fn build(self) -> Result<ProxyConfig, &'static str> {
Ok(ProxyConfig {
upstream_address: self.upstream.ok_or("Upstream not set")?.address,
upstream_port: self.upstream.ok_or("Upstream not set")?.port,
upstream_authority_pubkey: self.upstream.ok_or("Upstream not set")?.authority_pubkey,
downstream_address: self.downstream.ok_or("Downstream not set")?.address,
downstream_port: self.downstream.ok_or("Downstream not set")?.port,
max_supported_version: self.max_supported_version.ok_or("Max version not set")?,
min_supported_version: self.min_supported_version.ok_or("Min version not set")?,
min_extranonce2_size: self.min_extranonce2_size.ok_or("Min extranonce size not set")?,
downstream_difficulty_config: self.downstream.ok_or("Downstream not set")?.difficulty_config,
upstream_difficulty_config: self.upstream.ok_or("Upstream not set")?.difficulty_config,
})
}
}
So, the public API would look something like this:
let proxy_config = ProxyConfigBuilder::builder().upstream(upstream).downstream(downstream).max_supported_version(2).min_supported_version(1).min_extranonce2_size(8).build()
This makes it easier to set up and less likely to cause errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any cons in doing that. I just think that if we are going to use a specific pattern, it should be the same everywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this change is desired (I like builder patterns, personally), it should be a separate PR and issue, imho. Would make for a good first issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this change is desired (I like builder patterns, personally), it should be a separate PR and issue, imho. Would make for a
good first issue
Completely agree 💯
Maybe we could push this one which could be used as an example for the good first issue
(s)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I relayed #1116 to #1069 (comment)
this is a good idea but probably not too urgent for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also related: #1069 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm pending updating with base branch
let proxy_config = self.config.clone(); | ||
// Sender/Receiver to send a SV2 `SubmitSharesExtended` from the `Bridge` to the `Upstream` | ||
// (Sender<SubmitSharesExtended<'static>>, Receiver<SubmitSharesExtended<'static>>) | ||
let (tx_sv2_submit_shares_ext, rx_sv2_submit_shares_ext) = bounded(10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pub struct TranslatorProxyUpstream { | ||
address: String, | ||
port: u16, | ||
authority_pubkey: Secp256k1PublicKey, | ||
difficulty_config: UpstreamDifficultyConfig, | ||
} | ||
|
||
pub struct TranslatorProxyDownstream { | ||
address: String, | ||
port: u16, | ||
difficulty_config: DownstreamDifficultyConfig, | ||
} | ||
|
||
impl ProxyConfig { | ||
pub fn new( | ||
upstream: TranslatorProxyUpstream, | ||
downstream: TranslatorProxyDownstream, | ||
max_supported_version: u16, | ||
min_supported_version: u16, | ||
min_extranonce2_size: u16, | ||
) -> Self { | ||
Self { | ||
upstream_address: upstream.address, | ||
upstream_port: upstream.port, | ||
upstream_authority_pubkey: upstream.authority_pubkey, | ||
downstream_address: downstream.address, | ||
downstream_port: downstream.port, | ||
max_supported_version, | ||
min_supported_version, | ||
min_extranonce2_size, | ||
downstream_difficulty_config: downstream.difficulty_config, | ||
upstream_difficulty_config: upstream.difficulty_config, | ||
} | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this change is desired (I like builder patterns, personally), it should be a separate PR and issue, imho. Would make for a good first issue
} | ||
|
||
impl ProxyConfig { | ||
pub fn new( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like there could be additional limitation to the extranonce and version fields here. Definitely outside the scope of this PR but worth mentioning for discussion.
I tested the branch locally, and the logs for the translator look fine. However, I noticed quite a few warnings. I believe some of the error cases are no longer being triggered. If possible, could you remove those and resolve the other warnings as well? Everything else looks good. Warning Logs Translator
|
8461b4b
to
cb0ef3d
Compare
TranslatorSv2
lib code out of main.rs
TranslatorSv2
lib code out of main.rs
cb0ef3d
to
8d90263
Compare
Ill leave this to a different PR if thats ok? |
2447dac
to
43b9d01
Compare
43b9d01
to
6ac6768
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK.
But I left some comments, mostly due to warnings received while compiling it.
6ac6768
to
7664c9e
Compare
.. to allow using the `translator` in other environements.
7664c9e
to
1c5c317
Compare
Related to #1093