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

fix: Burn and Recover msgs public key checks and cli commands #13

Merged
merged 3 commits into from
Oct 9, 2024

Conversation

g-luca
Copy link
Contributor

@g-luca g-luca commented Sep 30, 2024

This PR addresses 2 issues in the upgraded implementation of the Burn and Recover messages:

  • Fixes the missing pub_key parameter in the CLI commands for both messages
  • Introduces a validation to check for nil values of the pub_key in the msg_server.go

@g-luca g-luca marked this pull request as ready for review October 7, 2024 08:36
@g-luca
Copy link
Contributor Author

g-luca commented Oct 7, 2024

Hey @gislik , the pr is ready for your review whenever you want 😄

Copy link
Member

@gislik gislik left a comment

Choose a reason for hiding this comment

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

Looks good 😊

I'm approving and I'll merge but I have one question, whether it makes sense to differentiate between invalid public keys and missing public keys?

I'll leave the decision up to you for a future PR if you think it's relevant.

@gislik gislik merged commit f2109c8 into monerium:main Oct 9, 2024
1 check passed
@g-luca
Copy link
Contributor Author

g-luca commented Oct 10, 2024

@gislik after all it's mostly about UX for the user/dev to determine the issue when the transaction fails. I think invalid should be enough to cover it, but if you prefer to distinguish between invalid and missing I'm happy to make that change, it's only a 3-line change 😄

@gislik
Copy link
Member

gislik commented Oct 11, 2024

I think it´s fine as is. I'm just trying to identify "rabbit holes", i.e. places where you spend more time debugging than you like to admit because you assume that the issue is one thing (signatures and their verification can be notoriously hard to get right) when the real issue is something much, much simpler (like the public key is just missing all together).

We'll always have the option of upgrading at a later stage if this becomes a real issue. So good to go 🚀

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.

3 participants