Show namespace in Type column of Transactions tab #482

JeremyRand · 2021-12-29T17:35:25Z

Eliminates ambiguous "name" wording in favor of "domain"/"identity"/etc. Also has the happy side effect of producing a scary "non-standard name" warning if someone tries to do a scam by selling a domain with whitespace at the end e.g. d/wikileaks .

Fixes #446

JeremyRand · 2021-12-29T19:13:15Z

@domob1812 This PR is ready for review.

domob1812

Thanks for the PR! I've added a few comments. Also - I think it would be good to have some unit tests for the new applications file.

domob1812 · 2021-12-30T08:47:51Z

src/names/applications.cpp

+NameNamespace
+NamespaceFromName (const std::string& name)
+{
+    NameNamespace purported = PurportedNamespaceFromName(name);


Nit: Can this be const?

Yes. Will fix this.

src/names/applications.cpp

domob1812 · 2021-12-30T08:49:16Z

src/names/applications.cpp

+            if (label.length() < 1)
+            {
+                return NameNamespace::NonStandard;
+            }


This check and the one above seem repeated for all cases. I suggest moving them before the switch to reduce code duplication.

Good point, will do that.

domob1812 · 2021-12-30T08:50:33Z

src/names/applications.cpp

+NameNamespace
+NamespaceFromName (const valtype& data)
+{
+    std::string name = std::string (data.begin (), data.end ());


Can you use DecodeName here instead? Even though it will not reduce code volume, it seems like the "proper" function to convert between valtype and std::string.

Agree in principle. I didn't do this because DecodeName requires a NameEncoding, and at this point in the code we don't yet know that the name is valid ASCII or UTF-8. Would it be okay to add a new NameEncoding::BINARY to the enum, which acts as a pass-through with no validation? This would simplify a lot of my other name GUI code too.

EDIT: Typo.

Why not just use UTF-8, which seems like a reasonable encoding in any case for a valid name?

In this particular case that might not hurt anything, but there are a bunch of other places in the name GUI code where I convert between valtype and std::string where binary data very well may show up (e.g. values of d/ names, which may in the future start using CBOR encoding). If we are agreed that "manual" conversion between those two types is undesirable for readability, I'd like to fix it everywhere, not just in the places where we are confident that the data is valid UTF-8.

I see. Well at the moment, basically valtype and std::string are not supposed to be directly convertible, but one is encoded while the other is not (similar to str and bytes in Python). So the conversion between them is using a particular encoding.

std::string is used when interfacing with the user, e.g. through RPC of the Qt UI. Are those places supposed to handle binary directly in the future?

I'm not sure I understand; std::string is not (AFAIK) aware of encoding; it's just a sequence of single-byte characters. In contrast, Python str is aware of encoding. You can put invalid UTF-8 into a std::string and nothing bad will happen. Is what you're describing just a non-standard convention used by Bitcoin Core to try to improve type-safety (in which case I guess I'm fine with abiding by it; just never ran into it before)?

This is just a convention I have in mind for the use of valtype vs std::string, based on how it evolved and how I think it makes sense for internal use especially with names/values. This can of course be changed if you have arguments for why you want to use std::string for binary values (but then the main question probably is - why not use valtype instead, which is basically what is meant for binary).

This sounds like a type-safety convention by another name, which I'm fine with. I will refactor this PR to avoid such conversions.

src/qt/transactiontablemodel.cpp

yanmaani · 2022-01-01T22:06:31Z

Shouldn't this be a RPC? See discussion in #365.

Why use ^ and $ in regex_match?

Note that regex_match will only successfully match a regular expression to an entire character sequence, whereas std::regex_search will successfully match subsequences.

I am not sure if manually parsing stuff by regex is the best way, or if simply providing a grammar and feeding it into a lexer/parser is better (in that case, we could simply use the grammar from the RFC). However, I am not necessarily opposed to regexes.

I am not sure if the API where you either get "good as X" or "invalid" is the best. I think I would prefer having two calls:

What's the namespace of d/mydomain? (A: d/)
Does d/mydomain comply with the rules for X namespace? (A: true)

There could also be a convenience call to combine them, but at least I think everything should be exposed.

yanmaani · 2022-01-01T22:13:41Z

Should phrases like "Non-standard name" really live in the QT code? Why not put them in common? They'd be useful for the RPC too, no?

Is it necessary to use IsStringValid, considering the later checks in the same code?

There are no unit tests.

JeremyRand · 2022-03-12T17:35:52Z

Shouldn't this be a RPC? See discussion in #365.

@yanmaani The names/applications.h API is both stateless and very high-level, which makes it different from things like name_update. As a result, it should be easy to test via unit tests (unlike name_update which requires functional tests), and there is not really any duplication incurred by having the Qt GUI call it directly instead of calling via RPC. I think it's reasonable to expose this to RPC users, but I don't think doing so is a blocker for adding it to the GUI.

Why use ^ and $ in regex_match?

Note that regex_match will only successfully match a regular expression to an entire character sequence, whereas std::regex_search will successfully match subsequences.

Fixed, thanks for catching that.

I am not sure if manually parsing stuff by regex is the best way, or if simply providing a grammar and feeding it into a lexer/parser is better (in that case, we could simply use the grammar from the RFC). However, I am not necessarily opposed to regexes.

The Namecoin specs use regex patterns, not grammars. So I think regex is fine here, as it makes it easier to audit whether they match the specs.

I am not sure if the API where you either get "good as X" or "invalid" is the best. I think I would prefer having two calls:
1. What's the namespace of `d/mydomain`? (A: `d/`)

2. Does `d/mydomain` comply with the rules for X namespace? (A: `true`)
There could also be a convenience call to combine them, but at least I think everything should be exposed.

The GUI only needs the combined call at the moment, and I am hesitant to expose API's that are not currently used anywhere. This PR does include an anonymous PurportedNamespaceFromName, so it should be trivial to expose that later if needed. I could probably refactor NamespaceFromName so that its validation routines are moved to a new anonymous IsNameValidForNamespace function (so we could trivially expose that function later as well); would you like me to do that?

JeremyRand · 2022-03-12T17:41:38Z

Should phrases like "Non-standard name" really live in the QT code? Why not put them in common? They'd be useful for the RPC too, no?

@yanmaani My understanding is that the static analyzer used by the localization scripts only works for the Qt GUI. So putting those strings in common only would break localization.

Is it necessary to use IsStringValid, considering the later checks in the same code?

Removed, thanks.

There are no unit tests.

No objection to adding unit tests prior to merging.

JeremyRand · 2022-03-12T20:36:06Z

Added unit tests. @domob1812 feel free to review again; I will squash before a merge.

domob1812

Thanks, looks good to me. I've added minor suggestions, but consider them optional. Let me know when squashed, then I can merge it.

src/names/applications.cpp

domob1812 · 2022-03-14T09:04:53Z

src/names/applications.cpp

+
+    const std::string label = name.substr(purportedLen);
+
+    if (label.length() < 1)


I would suggest using label.empty() instead, which seems a bit more expressive to me.

Thanks, will fix.

src/names/applications.cpp

JeremyRand · 2022-03-14T10:47:54Z

Addressed @domob1812 's latest round of review. Will wait a few days for @yanmaani to look over my responses to his review before I squash.

yanmaani · 2022-03-14T12:39:05Z

@yanmaani The names/applications.h API is both stateless and very high-level, which makes it different from things like name_update. As a result, it should be easy to test via unit tests (unlike name_update which requires functional tests), and there is not really any duplication incurred by having the Qt GUI call it directly instead of calling via RPC. I think it's reasonable to expose this to RPC users, but I don't think doing so is a blocker for adding it to the GUI.

Sure, if it's in a separate file I suppose it's fine.

Why use ^ and $ in regex_match?

Fixed, thanks for catching that.

As a nit, isn't it preferable to use regex_search so you can exactly copy the regex?

I am not sure if the API where you either get "good as X" or "invalid" is the best. I think I would prefer having two calls:
1. What's the namespace of `d/mydomain`? (A: `d/`)

2. Does `d/mydomain` comply with the rules for X namespace? (A: `true`)
The GUI only needs the combined call at the moment, and I am hesitant to expose API's that are not currently used anywhere. This PR does include an anonymous PurportedNamespaceFromName, so it should be trivial to expose that later if needed. I could probably refactor NamespaceFromName so that its validation routines are moved to a new anonymous IsNameValidForNamespace function (so we could trivially expose that function later as well); would you like me to do that?

I mean, the best would probably be if you could just call PurportedNamespaceFromName and then IsNameValidForNamespace inside the Qt code, that way both are used.

domob1812

Thanks, all good from my side now. Let me know when you think it is ready for merging.

JeremyRand · 2022-03-18T19:08:29Z

Why use ^ and $ in regex_match?

Fixed, thanks for catching that.

As a nit, isn't it preferable to use regex_search so you can exactly copy the regex?

@yanmaani I assume there is a performance benefit to using regex_match; and it seems pretty straightforward to audit anyway. I've added a comment that clarifies this.

I am not sure if the API where you either get "good as X" or "invalid" is the best. I think I would prefer having two calls:
1. What's the namespace of `d/mydomain`? (A: `d/`)

2. Does `d/mydomain` comply with the rules for X namespace? (A: `true`)
The GUI only needs the combined call at the moment, and I am hesitant to expose API's that are not currently used anywhere. This PR does include an anonymous PurportedNamespaceFromName, so it should be trivial to expose that later if needed. I could probably refactor NamespaceFromName so that its validation routines are moved to a new anonymous IsNameValidForNamespace function (so we could trivially expose that function later as well); would you like me to do that?
I mean, the best would probably be if you could just call PurportedNamespaceFromName and then IsNameValidForNamespace inside the Qt code, that way both are used.

Looking at the code some more, checking whether a name is valid for a given namespace requires checking its purported namespace anyway. So factoring out IsNameValidForNamespace wouldn't make much sense. I could factor out an IsLabelValidForNamespace (which accepts DOMAIN and wikileaks instead of DOMAIN and d/wikileaks), but at this point it feels way too much like we're exposing implementation details. So I think I prefer to leave it like it is, with the possibility of exposing PurportedNamespaceFromName in a future PR.

JeremyRand · 2022-03-24T01:18:32Z

@yanmaani Are there any remaining blockers here from your end?

JeremyRand · 2022-06-13T13:53:15Z

@domob1812 Squashed; feel free to merge.

This contains namecoin/namecoin-core#482, which handles specific namespaces. In the merge itself, the logic from Namecoin is still kept; it will be adapted to Xaya name types in a follow-up.

JeremyRand added enhancement qt labels Dec 29, 2021

JeremyRand force-pushed the qt-transactions-type-namespace branch 2 times, most recently from 59435cd to ff02537 Compare December 29, 2021 18:57

JeremyRand changed the title ~~(WIP) Show namespace in Type column of Transactions tab~~ Show namespace in Type column of Transactions tab Dec 29, 2021

JeremyRand mentioned this pull request Dec 29, 2021

Show .bit form for d/ names in TransactionRecord #483

Merged

domob1812 requested changes Dec 30, 2021

View reviewed changes

domob1812 approved these changes Mar 14, 2022

View reviewed changes

domob1812 approved these changes Mar 15, 2022

View reviewed changes

JeremyRand mentioned this pull request Mar 18, 2022

Explicitly use hex encoding in GUI to RPC API #484

Merged

JeremyRand added 2 commits June 13, 2022 13:37

Namecoin / Qt: Show namespace in Type column of Transactions tab

e52ebe5

Namecoin: Add name applications tests

add05e8

JeremyRand force-pushed the qt-transactions-type-namespace branch from 09b298e to add05e8 Compare June 13, 2022 13:51

domob1812 merged commit 3acb20f into namecoin:master Jun 14, 2022

JeremyRand mentioned this pull request Jan 4, 2023

Transactions tab's Type column should include namespace #446

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show namespace in Type column of Transactions tab #482

Show namespace in Type column of Transactions tab #482

JeremyRand commented Dec 29, 2021 •

edited

Loading

JeremyRand commented Dec 29, 2021

domob1812 left a comment

domob1812 Dec 30, 2021

JeremyRand Jan 3, 2022

JeremyRand Mar 12, 2022

domob1812 Dec 30, 2021

JeremyRand Jan 3, 2022

JeremyRand Mar 12, 2022

domob1812 Dec 30, 2021

JeremyRand Jan 3, 2022 •

edited

Loading

domob1812 Jan 4, 2022

JeremyRand Jan 4, 2022

domob1812 Jan 5, 2022

JeremyRand Jan 5, 2022

domob1812 Jan 5, 2022

JeremyRand Mar 12, 2022

JeremyRand Mar 12, 2022

yanmaani commented Jan 1, 2022

yanmaani commented Jan 1, 2022

JeremyRand commented Mar 12, 2022

JeremyRand commented Mar 12, 2022

JeremyRand commented Mar 12, 2022

domob1812 left a comment

domob1812 Mar 14, 2022

JeremyRand Mar 14, 2022

JeremyRand commented Mar 14, 2022

yanmaani commented Mar 14, 2022

domob1812 left a comment

JeremyRand commented Mar 18, 2022

JeremyRand commented Mar 24, 2022

JeremyRand commented Jun 13, 2022


		const std::string label = name.substr(purportedLen);

		if (label.length() < 1)

Show namespace in Type column of Transactions tab #482

Show namespace in Type column of Transactions tab #482

Conversation

JeremyRand commented Dec 29, 2021 • edited Loading

JeremyRand commented Dec 29, 2021

domob1812 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeremyRand Jan 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanmaani commented Jan 1, 2022

yanmaani commented Jan 1, 2022

JeremyRand commented Mar 12, 2022

JeremyRand commented Mar 12, 2022

JeremyRand commented Mar 12, 2022

domob1812 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeremyRand commented Mar 14, 2022

yanmaani commented Mar 14, 2022

domob1812 left a comment

Choose a reason for hiding this comment

JeremyRand commented Mar 18, 2022

JeremyRand commented Mar 24, 2022

JeremyRand commented Jun 13, 2022

JeremyRand commented Dec 29, 2021 •

edited

Loading

JeremyRand Jan 3, 2022 •

edited

Loading