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

Add signature_hash xfield and #36

Merged
merged 5 commits into from
Nov 29, 2023
Merged

Conversation

Naviabheeman
Copy link
Contributor

Add a new hash type called XfieldHash to get the hash of serialised xfield. This is used during tapyrus-signer setup to generate a threshold signature for new xfield changes.

src/blockdata/block.rs Show resolved Hide resolved
src/blockdata/block.rs Outdated Show resolved Hide resolved
src/blockdata/block.rs Outdated Show resolved Hide resolved
@Naviabheeman Naviabheeman requested a review from azuchi November 25, 2023 05:47
/// Return hash of serialized XField for signing
pub fn signature_hash(&self) ->Result<XFieldHash, Error> {
match self {
XField::None => Ok(XFieldHash::from_str("").unwrap()),
Copy link
Contributor

Choose a reason for hiding this comment

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

This code is intended to return a hash of an empty string, but it actually panics at XFieldHash::from_str("").unwrap(). And test case xfield_signature_hash_test_none also seems to expect that.

If you want to generate an error when None, XFieldHash::from_str("") is not necessary. On the other hand, if you return a hash for empty data, there should be no error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea was to return an empty string as the hash. But the hash constructor rejects it after checking the length. I never realised it as it doesn't happen in any real test scenario. I'll return error and fix the unit test.

@azuchi azuchi merged commit a2ed736 into chaintope:master Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants