qt: added verification of minimal json in the configure name dialog box #537

junekomeiji · 2024-03-27T08:38:25Z

I added checking of JSON minimality in names/applications.cpp alongside the associated unit tests and added them to the configure name dialog box. The dialog box should tell you if you inputted an non-minimal JSON value and warn you against configuring your domain with said invalid value, but should allow you to continue anyway if you were to insist, as well as allow you to minimalise said JSON value before writing it onto the blockchain.

JeremyRand · 2024-03-27T09:21:13Z

src/names/applications.cpp

+        return false;
+    } else {
+        std::string minimalJSON = GetMinimalJSON(text);
+        LogDebug(BCLog::NAMES, "Minimalised JSON string is: %s \n", minimalJSON);


I suspect that this log entry would be better if it were only done when the return value (see next line) is false, otherwise it may be a bit spammy. I'm not certain though; @domob1812 what do you think?

Yes indeed, I think this should only be logged (if at all) for when it fails the check.

This line should probably be indented now that it's within an if block?

Possible refactor idea: create a bool variable called matches, set it to text == minimalJSON, then you can avoid duplicating the comparison in the if and the return. I'm guessing that compiler optimizations will produce the same ASM either way, but I think it would be a bit more readable this way.

JeremyRand · 2024-03-27T09:23:18Z

src/qt/configurenamedialog.cpp

@@ -112,11 +143,17 @@ void ConfigureNameDialog::onDataEdited(const QString &name)
 {
    ui->dataSize->setText(tr("%1 / %2").arg(name.size()).arg(MAX_VALUE_LENGTH_UI));
    ui->dataSize->resize(ui->dataSize->fontMetrics().horizontalAdvance(ui->dataSize->text()), ui->dataSize->height());
-
+    


Can these added spaces be reverted?

JeremyRand · 2024-03-27T09:25:38Z

Nice work Rose! Code review of 1d20b47 looks good, modulo the two tiny things I noted above. Will do a build/test now and report back.

JeremyRand · 2024-03-27T09:39:47Z

Manual GUI testing of 1d20b47 is flawless AFAICT. Running the unit tests now....

JeremyRand · 2024-03-27T09:42:16Z

Unit tests also pass on 1d20b47.

I think this is mergeable once the above two trivial things are sorted out (the first one of which I'd like Daniel's opinion on before we decide on what to do).

domob1812

Thanks for the change, this definitely helps, too! I've just added a few minor things I think you should resolve before merging this.

src/names/applications.cpp

domob1812 · 2024-03-27T10:17:36Z

src/names/applications.cpp

+        return false;
+    } else {
+        std::string minimalJSON = GetMinimalJSON(text);
+        LogDebug(BCLog::NAMES, "Minimalised JSON string is: %s \n", minimalJSON);


Yes indeed, I think this should only be logged (if at all) for when it fails the check.

domob1812 · 2024-03-27T10:17:54Z

src/qt/configurenamedialog.cpp

@@ -62,6 +62,8 @@ void ConfigureNameDialog::accept()

    returnData = ui->dataEdit->text();
    returnTransferTo = ui->transferTo->text();
+
+


Is this an accidental change? Please revert it.

domob1812 · 2024-03-27T10:18:34Z

src/qt/configurenamedialog.cpp

+        if(reply == QMessageBox::AcceptRole){
+            QDialog::accept();
+        } else if(reply == QMessageBox::RejectRole){
+


This looks unfinished? Is this still WIP?

It's so it does nothing if a user clicks the Cancel button.

I think we can just leave out the RejectRole part of the if/else chain? If the intent is to make it clear that RejectRole is a possibility, I think a comment would be sufficient for that.

Yes I would suggest to either leave it out, or if you think it needs to be explicitly explained that we want to do nothing in that case, then leave it in but add a comment to that effect.

domob1812 · 2024-03-27T10:22:08Z

src/qt/configurenamedialog.cpp

-        ui->labelValidJSON->setText(tr("Valid JSON text."));
+    if(IsValidJSONOrEmptyString(data))
+    {
+        if(IsMinimalJSONOrEmptyString(data))


I would suggest to move this onto the top level, so it is not nested if's. What I would to to make the code more readable is to have an if-else construct on just one level, with these checks:

IsMinimalJSONOrEmptyString, which already checks for validity (i.e. if true, then the value is valid and minimal),

Else, IsValidJSONOrEmptyString, which would mean the value is valid but not minimal,

Else, the value is invalid JSON.

src/test/name_applications_tests.cpp

JeremyRand · 2024-03-28T05:41:16Z

src/names/applications.cpp

+
+    if(!v.read(text)){ 
+        return false;
+    } else {


Per Daniel's comment about the above else usage, I think you can also avoid else here since the if block includes a return.

JeremyRand · 2024-03-28T05:42:22Z

src/qt/configurenamedialog.cpp

@@ -62,7 +62,7 @@ void ConfigureNameDialog::accept()

    returnData = ui->dataEdit->text();
    returnTransferTo = ui->transferTo->text();
-
+    


Can you remove the spaces that were added on this line?

JeremyRand · 2024-03-28T05:45:26Z

src/qt/configurenamedialog.cpp

    } else {
        ui->labelValidJSON->setText(tr("JSON data inputted is invalid."));
    }
+


I think this blank line can be removed?

JeremyRand · 2024-03-28T17:48:56Z

ACK 5d41631. Unit tests pass, manual GUI testing works fine, code review looks good.

@domob1812 If this gets an ACK from you, is it OK if I merge to minimize delays?

domob1812

Looks good to me, please just consider the one more comment I added. Otherwise it is ready to squash and merge.

domob1812 · 2024-03-29T07:43:14Z

src/names/applications.cpp

+
+    std::string minimalJSON = GetMinimalJSON(text);
+
+    bool matches = (text == minimalJSON);


Both of minimalJSON and matches can be declared as const, and I think it is good style to do so in this case. Also I would rename matches maybe to isMinimal to make the code more readable.

JeremyRand · 2024-03-30T09:21:50Z

Code review of a6a991d looks good to me, testing now....

JeremyRand · 2024-03-30T09:33:56Z

ACK a6a991d; code review looks good, manual testing shows no issues, unit tests pass.

@domob1812 if this gets your ACK as well, is it OK if I merge it?

domob1812

ACK a6a991d. I can merge this myself now.

fe7c81e qt: change QLineEdit error background (w0xlt) Pull request description: This PR proposes a small change in QLineEdit when there is an error in the input. master | --- | ![image](https://user-images.githubusercontent.com/94266259/154762427-b816267e-ec70-4a8f-a7fb-1317ebacf1a4.png) PR | --- | ![image](https://user-images.githubusercontent.com/94266259/154761933-15eb3d81-ca81-4498-b8ec-cf1139ae2f8a.png) | This also shows good results when combined with other open PRs. namecoin#537 | --- | ![image](https://user-images.githubusercontent.com/94266259/154763411-6266a283-2d8a-4365-b3f2-a5cb545e773e.png) namecoin#533 | --- | ![image](https://user-images.githubusercontent.com/94266259/154765638-f38b13d6-a4f8-4b46-a470-f882668239f3.png) | ACKs for top commit: GBKS: ACK fe7c81e jarolrod: ACK fe7c81e shaavan: ACK fe7c81e Tree-SHA512: eccc53f42d11291944ccb96efdbe460cb10af857f1d4fa9b5348ddcb0796c82faf1cdad9040aae7a25c5d8f4007d6284eba868d7af14acf56280f6acae170b91

JeremyRand reviewed Mar 27, 2024

View reviewed changes

domob1812 reviewed Mar 27, 2024

View reviewed changes

JeremyRand reviewed Mar 28, 2024

View reviewed changes

domob1812 reviewed Mar 29, 2024

View reviewed changes

junekomeiji force-pushed the minimaljson branch from 5d41631 to 99f98a9 Compare March 29, 2024 21:52

qt: added verification of minimal json in the configure name dialog box

a6a991d

junekomeiji force-pushed the minimaljson branch from 99f98a9 to a6a991d Compare March 29, 2024 21:56

domob1812 approved these changes Apr 1, 2024

View reviewed changes

domob1812 merged commit 6d42a89 into namecoin:master Apr 1, 2024
13 of 16 checks passed

JeremyRand mentioned this pull request Apr 5, 2024

Configure Name dialog should indicate JSON compactness #509

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

qt: added verification of minimal json in the configure name dialog box #537

qt: added verification of minimal json in the configure name dialog box #537

junekomeiji commented Mar 27, 2024

JeremyRand Mar 27, 2024

domob1812 Mar 27, 2024

JeremyRand Mar 28, 2024

JeremyRand Mar 28, 2024

JeremyRand Mar 27, 2024

JeremyRand commented Mar 27, 2024

JeremyRand commented Mar 27, 2024

JeremyRand commented Mar 27, 2024

domob1812 left a comment

domob1812 Mar 27, 2024

domob1812 Mar 27, 2024

domob1812 Mar 27, 2024

junekomeiji Mar 27, 2024

JeremyRand Mar 28, 2024

domob1812 Mar 28, 2024

domob1812 Mar 27, 2024

JeremyRand Mar 28, 2024

JeremyRand Mar 28, 2024

JeremyRand Mar 28, 2024

JeremyRand commented Mar 28, 2024

domob1812 left a comment

domob1812 Mar 29, 2024

JeremyRand commented Mar 30, 2024

JeremyRand commented Mar 30, 2024

domob1812 left a comment

		@@ -62,6 +62,8 @@ void ConfigureNameDialog::accept()

		returnData = ui->dataEdit->text();
		returnTransferTo = ui->transferTo->text();

		@@ -62,7 +62,7 @@ void ConfigureNameDialog::accept()

		returnData = ui->dataEdit->text();
		returnTransferTo = ui->transferTo->text();


		std::string minimalJSON = GetMinimalJSON(text);

		bool matches = (text == minimalJSON);

qt: added verification of minimal json in the configure name dialog box #537

qt: added verification of minimal json in the configure name dialog box #537

Conversation

junekomeiji commented Mar 27, 2024

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 commented Mar 27, 2024

JeremyRand commented Mar 27, 2024

JeremyRand commented Mar 27, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeremyRand commented Mar 28, 2024

domob1812 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeremyRand commented Mar 30, 2024

JeremyRand commented Mar 30, 2024

domob1812 left a comment

Choose a reason for hiding this comment