namecoin / electrum-nmc

Namecoin port of Electrum Bitcoin client.
https://www.namecoin.org/
MIT License
29 stars 24 forks source link

Binary encoding #337

Closed robertmin1 closed 10 months ago

JeremyRand commented 1 year ago

It looks like this contains a copy of https://github.com/namecoin/electrum-nmc/pull/338 . Is that because there would be merge conflicts otherwise?

JeremyRand commented 1 year ago

I'd prefer not to commit compiled form files into Git; those files are already generated at build time, which is preferred by the Debian guys.

robertmin1 commented 1 year ago

Alright! I'm not sure why it has a copy of #338. I will check this out

robertmin1 commented 1 year ago

Fixed the two issues

JeremyRand commented 1 year ago

It looks like you're using bytes.fromhex etc. for messing with the encoding. We already have encoding/decoding functions in names.py that you can use for this, which will ensure consistency with Namecoin Core behavior.

robertmin1 commented 1 year ago

Switched to the encoding/decoding functions in names.py

JeremyRand commented 1 year ago

This looks pretty good so far.

Initial feedback:

  1. It might be useful to put the ASCII textbox and the hex textbox into tabs, so that the user only sees one at a time. You should be able to copy the tab GUI code from the Manage DNS dialog.
  2. The initial value of the hex textbox in the Buy Names tab is empty; it should get automatically filled in when the page loads, based on the default ASCII value.
  3. Some visual feedback if the user enters invalid hex strings (either non-hex characters or an odd number of hex characters) would be nice.

Keep it up! :)

JeremyRand commented 1 year ago

I tried typing hex that was invalid ASCII (e.g. ffffff) and the preview didn't update; I suspect that the preview is incorrectly retrieving its data from the ASCII textbox instead of the Hex textbox.

robertmin1 commented 1 year ago

A helper function might be helpful to handle the state of buttons and labels?

def update_buttons_and_label(self, condition=None, button=None, label=None, error_message=None):
    if button is not None:
        button.setEnabled(condition)
    if label is not None:
        if error_message:
            label.setText(error_message)
        else:
            label.clear()
JeremyRand commented 1 year ago

Is the idea to reuse that helper function across the Buy Names page, the Configure Name page, etc.? If so, yeah I think that's reasonable.

JeremyRand commented 1 year ago

Typing a string that doesn't end in .bit to the Domain textbox seems to cause the string to be copied verbatim to the ASCII textbox instead of producing an error.

JeremyRand commented 11 months ago

As discussed, no further changes are needed, but can you squash the commits, and then remove WIP: from the PR title?

JeremyRand commented 10 months ago

ACK 32af8d858c5a350d859728385c2c816b2621084d