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

(WIP) Explicitly use hex encoding in GUI to RPC API #484

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

(WIP) Explicitly use hex encoding in GUI to RPC API #484

wants to merge 2 commits into from

Conversation

JeremyRand
Copy link
Member

@JeremyRand JeremyRand commented Dec 30, 2021 •
edited

Prevents corruption issues that were happening when default encoding wasn't ASCII. Also a prereq to supporting binary data in the GUI.

@JeremyRand JeremyRand changed the title (WIP) Explicitly Use hex encoding in GUI to RPC API Explicitly Use hex encoding in GUI to RPC API Dec 30, 2021
@JeremyRand JeremyRand changed the title Explicitly Use hex encoding in GUI to RPC API Explicitly use hex encoding in GUI to RPC API Dec 30, 2021
@JeremyRand
Copy link
Member Author

@domob1812 This PR is ready for review.

domob1812
domob1812 requested changes Dec 30, 2021
View reviewed changes
Copy link

@domob1812 domob1812 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, but please consider the comments I've added. Also I think the commit history should be squashed - all into one commit seems appropriate here to me.

src/qt/buynamespage.cpp Outdated
params.pushKV ("name", strName);

valtype vtName = valtype (strName.begin (), strName.end ());
std::string hexName = EncodeName (vtName, NameEncoding::HEX);
Copy link

Choose a reason for hiding this comment

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

Maybe you could add a utility method that converts QString to hex directly and use it here as well as in other places? The conversion is simple, but I think having an explicit method like this would improve code readability a lot.

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems this code has the same type-safety issue that you brought up in #482 (comment) . Would it be okay if I refactored this so that the string argument is in hex form rather than binary form?

Copy link

Choose a reason for hiding this comment

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

Yes sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

I decided to defer this refactor until the GUI is updated to handle hex data, and instead just added a conversion function as you requested.

src/qt/nametablemodel.cpp Outdated
@@ -73,11 +75,17 @@ class NameTablePriv
// TODO: Set name and value encoding to hex, so that nonstandard
// encodings don't cause errors.
Copy link

Choose a reason for hiding this comment

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

Can this TODO be removed now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not yet; this PR uses hex encoding for talking to the RPC API but converts back to ASCII, so encoding errors will still be an issue for now.

Copy link

Choose a reason for hiding this comment

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

Ok, but then I think it should be reformulated. As it is currently, it makes no sense - the TODO says "set encodings to hex", but you actually do that in the very next line.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

src/qt/nametablemodel.cpp Outdated
const std::string data = maybeData.get_str();
const std::string hexName = maybeName.get_str();
const valtype vtName = DecodeName(hexName, NameEncoding::HEX);
const std::string name = std::string (vtName.begin (), vtName.end ());
Copy link

Choose a reason for hiding this comment

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

Same as above, I think an explicit utility method for this conversion would be useful and improve code readability.

@JeremyRand JeremyRand force-pushed the qt-hex-rpc branch 2 times, most recently from 3854aa4 to 4c4c539 Compare March 25, 2022 01:42
@JeremyRand
Copy link
Member Author

@domob1812 Addressed review, and fixed merge conflict via rebase. Feel free to proceed with review; I'll squash before you merge.

domob1812
domob1812 approved these changes Mar 28, 2022
View reviewed changes
Copy link

@domob1812 domob1812 left a comment

Choose a reason for hiding this comment

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

Thanks for the update, looks good. Please just consider updating the TODO text, otherwise this is good for me.

src/qt/nametablemodel.cpp Outdated
@@ -73,11 +75,17 @@ class NameTablePriv
// TODO: Set name and value encoding to hex, so that nonstandard
// encodings don't cause errors.
Copy link

Choose a reason for hiding this comment

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

Ok, but then I think it should be reformulated. As it is currently, it makes no sense - the TODO says "set encodings to hex", but you actually do that in the very next line.

@JeremyRand
Copy link
Member Author

@domob1812 Review addressed; let me know if this is ready for a squash.

domob1812
domob1812 approved these changes Jun 14, 2022
View reviewed changes
Copy link

@domob1812 domob1812 left a comment

Choose a reason for hiding this comment

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

Thanks for the updates. It looks good now, please just consider merging the two try-catch blocks in both places. Then it is good to merge for me.

src/qt/nametablemodel.cpp Show resolved Hide resolved
src/qt/nametablemodel.cpp Show resolved Hide resolved
@JeremyRand
Copy link
Member Author

@domob1812 Review addressed once again; let me know if this is ready for a squash.

@JeremyRand JeremyRand mentioned this pull request Jan 5, 2023
Open
domob1812
domob1812 approved these changes Jan 9, 2023
View reviewed changes
Copy link

@domob1812 domob1812 left a comment

Choose a reason for hiding this comment

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

ACK, I think this is ready for squashing and merging.

@JeremyRand
Copy link
Member Author

Rebased to fix merge conflict. I still need to confirm that the rebase didn't break anything, will test that in the next day or two.

Namecoin / Qt: don't hide name ops sent to change addresses
21fe6d3 
This particularly matters because the name_new RPC method sends to a
change address by default, so if you issued a name_new, it would never
show up in the Transactions tab.
@JeremyRand
Copy link
Member Author

Blocked by #536

@JeremyRand JeremyRand changed the title Explicitly use hex encoding in GUI to RPC API (WIP) Explicitly use hex encoding in GUI to RPC API Mar 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@domob1812 domob1812 domob1812 approved these changes

Assignees
No one assigned
Labels
enhancement qt
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@JeremyRand @domob1812