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

Allowing the creation of height locked Slates Fixes #564 #565

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Allowing the creation of height locked Slates Fixes #564 #565

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cbfd098
Allowing the creation of height locked Slates Fixes #564
Jan 5, 2021
08bd462
[Dev] panic in case of invalid kernel_feat_param #PR-564
Mar 19, 2021
a896b19
[Doc] Updated comment to better reflect behavior of
Mar 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 31 additions & 3 deletions libwallet/src/slate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,8 +250,21 @@ impl Slate {
tx
}

/// Create a new slate
/// Create a new slate with a plain kernel
pub fn blank(num_participants: u8, is_invoice: bool) -> Slate {
Slate::blank_with_kernel_features(num_participants, is_invoice, 0, None)
}

/// 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)
Copy link
Member

Choose a reason for hiding this comment

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

I think we should update this comment as well now that we made it panic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

very true, thanks! Corrected the comment now.

pub fn blank_with_kernel_features(
num_participants: u8,
is_invoice: bool,
kernel_feat_param: u8,
lock_height_param: Option<u64>,
) -> Slate {
Comment on lines +262 to +267
Copy link
Member

Choose a reason for hiding this comment

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

Would that makes sense to pass a kernel_features like that?

Suggested change
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.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with this suggestion, doing it in this way also doesn't need code modification if/when we add new kernel features 👍

Copy link
Member

Choose a reason for hiding this comment

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

@jafalter pinging just in case it was forgotten

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@jafalter jafalter Apr 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@quentinlesceller @phyro the Slate struct requires a Option<KernelFeaturesArgs> see:
image
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.

Copy link
Member

Choose a reason for hiding this comment

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

@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

let np = match num_participants {
0 => 2,
n => n,
Expand All @@ -260,6 +273,20 @@ impl Slate {
true => SlateState::Invoice1,
false => SlateState::Standard1,
};
let kernel_features = if kernel_feat_param > 3 {
0
Copy link

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed, fixed

} else {
kernel_feat_param
};
let is_height_locked =
lock_height_param.is_some() && (kernel_features == 2 || kernel_features == 3);
let kernel_feat_args = if is_height_locked {
Some(KernelFeaturesArgs {
lock_height: lock_height_param.unwrap(),
})
} else {
None
};
Slate {
num_participants: np, // assume 2 if not present
id: Uuid::new_v4(),
Expand All @@ -268,17 +295,18 @@ impl Slate {
amount: 0,
fee_fields: FeeFields::zero(),
ttl_cutoff_height: 0,
kernel_features: 0,
kernel_features: kernel_features,
offset: BlindingFactor::zero(),
participant_data: vec![],
version_info: VersionCompatInfo {
version: CURRENT_SLATE_VERSION,
block_header_version: GRIN_BLOCK_HEADER_VERSION,
},
payment_proof: None,
kernel_features_args: None,
kernel_features_args: kernel_feat_args,
}
}

/// Removes any signature data that isn't mine, for compacting
/// slates for a return journey
pub fn remove_other_sigdata<K>(
Expand Down