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

feat(iroh): add ticket prefixes and a doctor ticket-inspect command #1711

Merged
merged 19 commits into from
Oct 26, 2023

Conversation

divagant-martian
Copy link
Contributor

@divagant-martian divagant-martian commented Oct 23, 2023

Description

Adds a doc: and blob: prefix to the doc and blob tickets respectively. This is mandatory and identifies the kind of iroh ticket.

Notes & open questions

doctor sample run (happy path):

doctor ticket-inspect blob:edjoqrkky753mdwphqrxbr2wjzkizv4hkb5s5ovav73c2zfuaja7aaibaqalkpj5jcqzsaqawu6t2sfbvubqbqfiaaf4ivyaycuaadgek4adtjetziwzcpcdqzemuxmwei2fp5wtmfjcy4qysovyzjhahfao3aaa
Blob ticket:
Ticket {
    node: PeerAddr {
        peer_id: PublicKey(2luekswh7o3a5tz4),
        info: AddrInfo {
            derp_region: Some(
                1,
            ),
            direct_addresses: {
                181.61.61.72:34144,
                181.61.61.72:61486,
                192.168.0.12:11204,
                192.168.0.25:11204,
            },
        },
    },
    format: Raw,
    hash: Hash(
        39a493ca2d913c438648ca5d96223457f6d361522c721893ab8ca4e03940ed80,
    ),
    token: None,
}

doctor sample run (unhappy path):

 doctor ticket-inspect blob:EDJOQRKKY753MDWPHQRXBR2WJZKIZV4HKB5S5OVAV73C2ZFUAJA7AAIBAQALKPJ5JDQIUAQAWU6T2SFO4ABQBQFIAAGMIVYAYCU'
Error: failed parsing blob ticket

Caused by:
    0: decoding failed: invalid length at 98
    1: invalid length at 98

another unhappy path:

doctor ticket-inspect asds:EDJOQRKKY753MDWPHQRXBR2WJZKIZV4HKB5S5OVAV73C2ZFUAJA7AAIBAQALKPJ5JDQIUAQAWU6T2SFO4ABQBQFIAAGMIVYAYCUAAGOEK4ADTJETZIWZCPCDQZEMUXMWEI2FP5WTMFJCY4QYSOVYZJHAHFAO3AAA'
Error: unrecogized ticket prefix

Caused by:
    Matching variant not found

Change checklist

  • Self-review.
  • Documentation updates if relevant.
  • Tests if relevant.

@divagant-martian divagant-martian requested a review from b5 October 23, 2023 22:27
@divagant-martian divagant-martian marked this pull request as draft October 24, 2023 18:22
@divagant-martian divagant-martian marked this pull request as ready for review October 24, 2023 23:02
@divagant-martian divagant-martian changed the title add ticket prefixes feat(iroh, doctor): add ticket prefixes and a doctor ticket-inspect command Oct 24, 2023
@divagant-martian divagant-martian changed the title feat(iroh, doctor): add ticket prefixes and a doctor ticket-inspect command feat(iroh): add ticket prefixes and a doctor ticket-inspect command Oct 24, 2023
/// Serialize to string.
fn serialize(&self) -> String {
let mut out = Self::KIND.to_string();
out.push(':');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I want to propose we use _ instead of : to separate. my reason: I can't double-click the ticket to get the entire string if we use the colon. If we use the underscore it'll work

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious I thought this would happen but weirdly it works on my terminal

const KIND: Kind = Kind::Blob;
}

impl std::str::FromStr for Ticket {
Copy link
Contributor

@dignifiedquire dignifiedquire Oct 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couldn't this be done with a blanket impl

impl<T: Ticket> FromStr for T {
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sadly no, due to orphan rules we need to own either the type over which the trait is implemented, or the implemented trait, and here we only own the bound

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:(

}

/// Deserializes from bytes.
pub fn from_bytes(bytes: &[u8]) -> Result<Self> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shadowing of method names is a bit tricky. Maybe add a verify_ticket method to the trait (which is called when doing from_bytes, with a default impl of Ok(()) and implement the verification you are doing here with that?

@dignifiedquire
Copy link
Contributor

contains an empty file of node.rs that can be removed

@divagant-martian
Copy link
Contributor Author

divagant-martian commented Oct 26, 2023

in light of the changes done in #1738 I'm thinking there is no need to group the tickets together so not to overwrite the authorship of the code. What would you prefer @dignifiedquire to keep them together like this or where they currently are on main? Revert to where they are should be quick
EDIT: did the changes and it's a bit harder to follow the impls if not kept together

@dignifiedquire
Copy link
Contributor

I think it is great to move them together

@divagant-martian divagant-martian added this pull request to the merge queue Oct 26, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 26, 2023
@divagant-martian divagant-martian added this pull request to the merge queue Oct 26, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 26, 2023
@divagant-martian divagant-martian added this pull request to the merge queue Oct 26, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 26, 2023
@divagant-martian divagant-martian added this pull request to the merge queue Oct 26, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 26, 2023
@divagant-martian divagant-martian added this pull request to the merge queue Oct 26, 2023
Merged via the queue into n0-computer:main with commit 2d292e3 Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

doc & iroh start tickets are easy to get mixed up, present useless error
3 participants