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.

Sign up for GitHub

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

Jump to bottom

add priority canonical block #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add priority canonical block #3

wants to merge 1 commit into from

Conversation

dernatsch
Copy link

gh0st42
gh0st42 requested changes Mar 20, 2023
View reviewed changes
Copy link
Member

@gh0st42 gh0st42 left a comment

Choose a reason for hiding this comment

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

thanks for the PR, some more documentation would be nice.
also a complete example on how to create a bundle and add the priority block would be a good thing for the documentation of the whole module.

src/priority/mod.rs
InvalidPriorityBlock,
}

// HOP_COUNT_BLOCK is a BlockType for a Hop Count block as defined in
Copy link
Member

Choose a reason for hiding this comment

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

could you please add a comment describing the block a bit better?
don't follow my bad example :)

src/priority/mod.rs
pub const PRIORITY_BLOCK: CanonicalBlockType = 224;

#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
pub struct PriorityBlockData(u16);
Copy link
Member

Choose a reason for hiding this comment

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

a doc string describing the priority system would be nice. is lower better or higher?
it might make sense to have const's or enums for HIGHEST, LOWEST and MEDIUM priorities.
an even more convenient way for the API would be helpers to return a complete block, e.g., new_highest_priority_block, new_lowest_priority_block, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gh0st42 gh0st42 gh0st42 requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dernatsch @gh0st42