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

refactor: make pubkey optional for decoded_json in keystore in decode_str! function #1104

Merged
merged 6 commits into from
May 23, 2024

Conversation

artemd24
Copy link
Contributor

@artemd24 artemd24 commented May 22, 2024

Motivation

There is not need to raise exception because of cheking pub keys further

Description

Change Map.fetch!(decoded_json, "pubkey") |> parse_binary!() to

pubkey =
      case Map.has_key?(decoded_json, "pubkey") do
        true -> Map.get(decoded_json, "pubkey") |> parse_binary!()
        false -> derived_pubkey
      end

+some tests added

Resolves #1084

Closes #1084

@artemd24 artemd24 requested a review from a team as a code owner May 22, 2024 20:04
Copy link
Collaborator

@MegaRedHand MegaRedHand left a comment

Choose a reason for hiding this comment

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

Hey there @artemd24! Thanks for your contribution!

As part of this issue, we need to handle when the "pubkey" field isn't specified. In that case, we need to ignore the following comparison against derived_pubkey, and set pubkey = derived_pubkey. Also, please add unit tests to this change (you can find examples in test/unit/keystore_test.exs).

@artemd24
Copy link
Contributor Author

Hello @MegaRedHand!

I have actualized changes according to your comment.
If you have more improvement ideas, please write it, I'll make it up with pleasure!

test/unit/keystore_test.exs Outdated Show resolved Hide resolved
test/unit/keystore_test.exs Outdated Show resolved Hide resolved
@MegaRedHand
Copy link
Collaborator

Great work @artemd24 ! There seem to be some formatting errors. Please run mix format.

@artemd24
Copy link
Contributor Author

@MegaRedHand thank you for helping me!
If you have more good first issues I will be glad to contribute

Copy link
Collaborator

@MegaRedHand MegaRedHand left a comment

Choose a reason for hiding this comment

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

Thanks again for your contribution! 😄

@MegaRedHand MegaRedHand enabled auto-merge (squash) May 23, 2024 20:59
@MegaRedHand MegaRedHand merged commit 9285a71 into lambdaclass:main May 23, 2024
13 checks passed
@MegaRedHand
Copy link
Collaborator

@all-contributors please add @artemd24 for code

Copy link
Contributor

@MegaRedHand

I've put up a pull request to add @artemd24! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Make pubkey field optional
2 participants