Allowing the creation of height locked Slates Fixes #564#565
Conversation
DavidBurkett
left a comment
DavidBurkett
left a comment
There was a problem hiding this comment.
This change looks good to me. @antiochp @quentinlesceller, can one of you merge this?
| false => SlateState::Standard1, | ||
| }; | ||
| let kernel_features = if kernel_feat_param > 3 { | ||
| 0 |
There was a problem hiding this comment.
my guess is that kernel_feat_param being > 3 means there's a bug, so is it possible that a panic would be better in this case?
|
@antiochp @quentinlesceller do you guys think this one is good to merge? |
| /// Create a new slate with custom kernel_feature and lock height | ||
| /// If a lock_height is set kernel_features will have to be either HeightLocked (2) or NoRecentDuplicate (3) | ||
| /// otherwise the value passed as lock_height will be ignored and a plain kernel will be created | ||
| /// Invalid arguments for kernel_features (>3) will be set to 0 (Plain) |
There was a problem hiding this comment.
I think we should update this comment as well now that we made it panic
There was a problem hiding this comment.
very true, thanks! Corrected the comment now.
blank_with_kernel_features mimblewimble#564
|
alright, @antiochp @quentinlesceller could one of you two also give it a quick look now and merge if it looks good? |
quentinlesceller
left a comment
quentinlesceller
left a comment
There was a problem hiding this comment.
Thank you for the PR @jafalter! I have a question regarding safety and possible simplification.
| pub fn blank_with_kernel_features( | ||
| num_participants: u8, | ||
| is_invoice: bool, | ||
| kernel_feat_param: u8, | ||
| lock_height_param: Option<u64>, | ||
| ) -> Slate { |
There was a problem hiding this comment.
Would that makes sense to pass a kernel_features like that?
| pub fn blank_with_kernel_features( | |
| num_participants: u8, | |
| is_invoice: bool, | |
| kernel_feat_param: u8, | |
| lock_height_param: Option<u64>, | |
| ) -> Slate { | |
| pub fn blank_with_kernel_features( | |
| num_participants: u8, | |
| is_invoice: bool, | |
| kernel_features: KernelFeatures, | |
| ) -> Slate { |
That would avoid to have the whole parsing and possible panic in here.
There was a problem hiding this comment.
I agree with this suggestion, doing it in this way also doesn't need code modification if/when we add new kernel features 👍
There was a problem hiding this comment.
@jafalter pinging just in case it was forgotten
There was a problem hiding this comment.
Thanks, I will do that. But I just need to doublecheck because I remember one of the types was not publicly visible I think that is why I did it like that.
There was a problem hiding this comment.
@quentinlesceller @phyro the Slate struct requires a Option<KernelFeaturesArgs> see:
KernelFeaturesArgs is not a publicly visible struct, therefore can't be passed as the parameter. Using KernelFeatures as you recommended would require bigger refactoring of the Slate struct, this is why I went for this solution.
There was a problem hiding this comment.
@jafalter sorry for the late response. I see, in this case I'm ok if we go this way and we can possibly refactor this later 👍
cc: @quentinlesceller
|
is that forgotten? |
6 participants
Created new function
blank_with_kernel_featureswhich takes (compared to the regularblank(num_participants: u8, is_invoice: bool))the additional parameterskernel_feat_param: u8,andlock_height_param: Option<u64>such that one can create a slate with a height locked kernel.This fixes the issue #564
I have adjusted
blank(num_participants: u8, is_invoice: bool)to use this new function to avoid code duplication.