- 
                Notifications
    You must be signed in to change notification settings 
- Fork 38
feat: allow specifying custom relay in dumbpipe (#60) #80
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
base: main
Are you sure you want to change the base?
Conversation
        
          
                Cargo.toml
              
                Outdated
          
        
      | tracing-subscriber = { version = "0.3.18", features = ["env-filter"] } | ||
| data-encoding = "2.9.0" | ||
| n0-snafu = "0.2.1" | ||
| anyhow = "1.0.99" | 
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 doesn't look like anyhow is used in your PR?
Edit: disregard, sorry, I see it now
        
          
                src/main.rs
              
                Outdated
          
        
      | } | ||
|  | ||
| impl FromStr for RelayModeOption { | ||
| type Err = anyhow::Error; | 
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.
Could the error type here be iroh::RelayUrlParseError?
| would be nice to avoid the  | 
4339481    to
    5728467      
    Compare
  
    | Ok, thank you for the feedback, I hadn't seen this RelayUrlParseError. I'll look into the testing soon then and come back to you 👍 | 
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.
When running with a relay disabled, this call to home_relay().initialized()  never returns, and so a ticket is never printed
| This looks pretty good, can you fix the lint issues and move it to ready (it is still marked as draft)? | 
| Actually, as @eminence says, when running dumbpipe with the  I've added a condition to not run this check if we disable the use of a relay. And I noticed that this check is not made for the  Also, the same issue is present on Sendme. Beside that I added some tests and info in the readme. | 
| /// The relay URL to use as a home relay, | ||
| /// | ||
| /// Can be set to "disabled" to disable relay servers and "custom" | ||
| /// to configure custom servers. The default is the n0 quickest responding | ||
| /// relay if the flag is not set. | ||
| #[clap(long, default_value_t = RelayModeOption::Default)] | ||
| pub relay: RelayModeOption, | 
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.
This help text needs a little tweaking I think.
You don't set this option to the literal value of "custom", but rather you give your relay url
ac4e882    to
    d4ed3d4      
    Compare
  
    | Hello! I've implemented the timeout like @dignifiedquire did in sendme. Tell me if there is anything else I should change on this pr. | 
231dbbc    to
    000c70b      
    Compare
  
    000c70b    to
    e02dcd3      
    Compare
  
            
          
                src/main.rs
              
                Outdated
          
        
      | let _ = tokio::time::timeout(Duration::from_secs(30), async { | ||
| if !(common.relay == RelayModeOption::Disabled) { | ||
| endpoint.online().await; | ||
| } | ||
| }).await; | 
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 a little weird to put the if-conditional inside the timeout. I would put the if-conditional on the outside.
Also, the 0.93.0 blog post recommends a value of 5 seconds, which is what I've implemented in my PR here
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.
Ok thank you for the resource!
I've made the modifs.
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.
Btw I'm not sure to understand why just let _ = the Result of the timeout instead of .expect() or something to stop if the endpoint does get online.
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.
Maybe a warning?
if !(common.relay == RelayModeOption::Disabled)
  && tokio::time::timeout(Duration::from_secs(5), endpoint.online()).await.is_err() {
    eprintln!("Error waiting for relay connection (timeout)");
  };c9fbac3    to
    1b523fe      
    Compare
  
    
Hello I'm back!
I thought this issue would be a good first step to get familiar with Iroh, so I started implementing the possibility to define a custom relay.
As mentioned in the issue, Sendme already supports this, so I mostly adapted that code.
Before going further and adding tests, I’d like your feedback on a few points:
anyhowdependency like in Sendme. Would you prefer a different error-handling approach? I could just implement the From<> myself.RelayModeintoRelayModeOption, which removes theStagingoption. I haven’t really looked into how staging works, would you like me to explore that for this implementation?