Polygon for DNS resolution #13202

supermassive · 2022-04-28T12:23:59Z

Resolves brave/brave-browser#22147
Resolves brave/brave-browser#22151

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

These domains could be used to test polygon DNS resolution
uns-devtest-fda7a9.x (transaction)
uns-devtest-brave.x(transaction)
both should redirect to https://brave.com/

kdenhartog

Few comments for the sec team, but I didn't see a security review issue opened for this yet, but it looks like we're code owners on some of the changed files.

browser/net/decentralized_dns_network_delegate_helper_unittest.cc

chromium_src/net/dns/dns_transaction.cc

components/brave_wallet/browser/brave_wallet_utils.cc

components/brave_wallet/browser/json_rpc_service.cc

components/brave_wallet/browser/json_rpc_service_test_utils.cc

components/brave_wallet/browser/json_rpc_service_test_utils_unittest.cc

net/decentralized_dns/constants.h

components/brave_wallet/browser/brave_wallet_utils.cc

components/brave_wallet/browser/unstoppable_domains_dns_resolve.cc

components/brave_wallet/browser/brave_wallet_utils.cc

components/brave_wallet/browser/unstoppable_domains_multichain_calls.h

chromium_src/net/base/lookup_string_in_fixed_set.cc

diracdeltas · 2022-05-11T04:26:41Z

does https://github.com/brave/brave-browser/wiki/Resolve-Methods-for-Unstoppable-Domains need an update here?

also could you remind me what is the expected behavior in Tor windows?

supermassive · 2022-05-11T04:54:11Z

does https://github.com/brave/brave-browser/wiki/Resolve-Methods-for-Unstoppable-Domains need an update here?

There are some issues in parallel regarding deprecation of DNS over HTTPS UD resolver:
brave/brave-browser#22148
brave/brave-browser#22149
Was going to update wiki as a part of this
#13256 (comment)

also could you remind me what is the expected behavior in Tor windows?

UD and ENS resolvers are disabled for incognito browsers:

brave-core/browser/net/decentralized_dns_network_delegate_helper.cc

Line 27 in 04732b0

ctx->browser_context->IsOffTheRecord() || !g_browser_process) {

iefremov · 2022-05-11T21:47:07Z

components/brave_wallet/browser/brave_wallet_constants.h

@@ -622,6 +622,8 @@ const char kAffiliateAddress[] = "0xbd9420A98a7Bd6B89765e5715e169481602D9c3d";

 const int64_t kBlockTrackerDefaultTimeInSeconds = 20;

+const char kPolygonMainnetEndpoint[] = "https://mainnet-polygon.brave.com/";


added many of them
7d207e0

chromium_src/net/dns/dns_transaction.cc

goodov · 2022-05-12T10:13:55Z

browser/net/decentralized_dns_network_delegate_helper.cc

@@ -92,37 +82,11 @@ void OnBeforeURLRequest_EnsRedirectWork(
 void OnBeforeURLRequest_UnstoppableDomainsRedirectWork(
    const brave::ResponseCallback& next_callback,
    std::shared_ptr<brave::BraveRequestInfo> ctx,


I think we should refactor std::shared_ptr out. not in this PR, but as a separate issue.
cc @iefremov

actually it has been with us for so long that I'm thinking of keeping it as a memorial
(just kidding)

goodov · 2022-05-12T10:29:37Z

browser/net/decentralized_dns_network_delegate_helper_unittest.cc

+      "https://brave",
+      "https://brave.com",
+      "https://brave.zil",
+  };


the test could be simplified with struct TestCase { domain, is_valid } to keep only one for and altering an expected value inside.

goodov · 2022-05-12T10:56:03Z

components/brave_wallet/browser/unstoppable_domains_dns_resolve.cc

+// for more details.
+const constexpr char* const kRecordKeys[] = {
+    "dweb.ipfs.hash", "ipfs.html.value",      "dns.A",
+    "dns.AAAA",       "browser.redirect_url", "ipfs.redirect_domain.value"};


a trailing comma can make such lists look better (and in-sync with the enum above).

goodov · 2022-05-12T10:56:12Z

components/brave_wallet/browser/unstoppable_domains_dns_resolve.cc

+    "dweb.ipfs.hash", "ipfs.html.value",      "dns.A",
+    "dns.AAAA",       "browser.redirect_url", "ipfs.redirect_domain.value"};
+static_assert(static_cast<size_t>(RecordKeys::RECORD_KEY_COUNT) ==
+                  sizeof(kRecordKeys) / sizeof(kRecordKeys[0]),


std::size(kRecordKeys) should do the trick (it's now allowed).

goodov · 2022-05-12T10:58:01Z

components/brave_wallet/browser/unstoppable_domains_dns_resolve.cc

+namespace brave_wallet::unstoppable_domains {
+
+namespace {
+enum RecordKeys {


enum class

kept plain enum so there are no static_casts when used as index in vector

goodov · 2022-05-12T11:02:03Z

components/brave_wallet/browser/unstoppable_domains_dns_resolve.cc

+  DNS_AAAA,
+  BROWSER_REDIRECT_URL,
+  IPFS_REDIRECT_DOMAIN_VALUE,
+  RECORD_KEY_COUNT,


kStyled constants are preferred around Chromium. see also brave_wallet_types.h.
https://google.github.io/styleguide/cppguide.html#Enumerator_Names

goodov

chromium_src lgtm, but some other parts can be improved.

supermassive · 2022-05-12T12:25:54Z

@diracdeltas @kdenhartog PTAL

goodov · 2022-05-12T12:26:26Z

components/brave_wallet/browser/unstoppable_domains_dns_resolve.cc

-    "dns.AAAA",       "browser.redirect_url", "ipfs.redirect_domain.value"};
-static_assert(static_cast<size_t>(RecordKeys::RECORD_KEY_COUNT) ==
-                  sizeof(kRecordKeys) / sizeof(kRecordKeys[0]),
+    "dns.AAAA",       "browser.redirect_url", "ipfs.redirect_domain.value",


huh, I was expecting that it will become a one column thing :)

It seems it doesn't work like that :D

if you add one more item then it will. I guess there's some logic "if it fits into two rows, keep it, otherwise do a column".

kdenhartog

Thanks, my concerns have been addressed. Looks good to me

EDIT: checked with Yan as well and she's good with this going forward. LGTM

supermassive requested review from a team and iefremov as code owners April 28, 2022 12:24

github-actions bot assigned supermassive Apr 28, 2022

github-actions bot added the CI/run-network-audit Run network-audit label Apr 28, 2022

supermassive force-pushed the PolygonDNSResolution branch 2 times, most recently from c82bc9a to d684445 Compare April 29, 2022 04:18

supermassive added this to the 1.40.x - Nightly milestone Apr 29, 2022

supermassive force-pushed the PolygonDNSResolution branch 4 times, most recently from 57dac57 to 632878d Compare April 29, 2022 06:50

supermassive changed the title ~~[WIP] Polygon for DNS resolution~~ Polygon for DNS resolution Apr 29, 2022

yrliou self-requested a review May 3, 2022 16:01

supermassive force-pushed the PolygonDNSResolution branch 2 times, most recently from 6a730ce to 04732b0 Compare May 5, 2022 12:25

supermassive requested a review from a team as a code owner May 5, 2022 12:25

kdenhartog reviewed May 6, 2022

View reviewed changes

supermassive force-pushed the PolygonDNSResolution branch from bda3d8f to 5126db2 Compare May 10, 2022 07:16

yrliou approved these changes May 10, 2022

View reviewed changes

diracdeltas reviewed May 11, 2022

View reviewed changes

chromium_src/net/base/lookup_string_in_fixed_set.cc Outdated Show resolved Hide resolved

supermassive mentioned this pull request May 11, 2022

Do not use selected network when resolving UD/ENS domains to eth address brave/brave-browser#22151

Closed

supermassive force-pushed the PolygonDNSResolution branch from 4f91656 to 80159b4 Compare May 11, 2022 06:35

iefremov reviewed May 11, 2022

View reviewed changes

chromium_src/net/dns/dns_transaction.cc Outdated Show resolved Hide resolved

iefremov approved these changes May 11, 2022

View reviewed changes

goodov reviewed May 12, 2022

View reviewed changes

goodov approved these changes May 12, 2022

View reviewed changes

supermassive added 6 commits May 12, 2022 18:14

Polygon for DNS resolution

662c2a7

ENS fixes and polygon proxy

fce2dbd

fix per review

4f0b4ab

review fixes

4d7203d

review fixes

631abf6

review fix

c07d7cb

supermassive force-pushed the PolygonDNSResolution branch from 7d207e0 to c07d7cb Compare May 12, 2022 12:16

goodov reviewed May 12, 2022

View reviewed changes

fix lint

5755c56

kdenhartog approved these changes May 12, 2022

View reviewed changes

supermassive merged commit 5e8ca8a into master May 13, 2022

supermassive deleted the PolygonDNSResolution branch May 13, 2022 06:52

srirambv mentioned this pull request May 27, 2022

Unstoppable Domains layer 2 support for resolving domains in URL bar brave/brave-browser#22147

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Polygon for DNS resolution #13202

Polygon for DNS resolution #13202

supermassive commented Apr 28, 2022 •

edited

Loading

kdenhartog left a comment

diracdeltas commented May 11, 2022

supermassive commented May 11, 2022

iefremov May 11, 2022

supermassive May 12, 2022

goodov May 12, 2022

iefremov May 12, 2022

goodov May 12, 2022

goodov May 12, 2022

goodov May 12, 2022

goodov May 12, 2022

supermassive May 12, 2022

goodov May 12, 2022

goodov left a comment

supermassive commented May 12, 2022

goodov May 12, 2022 •

edited

Loading

supermassive May 12, 2022

goodov May 12, 2022

kdenhartog left a comment •

edited

Loading

		@@ -622,6 +622,8 @@ const char kAffiliateAddress[] = "0xbd9420A98a7Bd6B89765e5715e169481602D9c3d";

		const int64_t kBlockTrackerDefaultTimeInSeconds = 20;

		const char kPolygonMainnetEndpoint[] = "https://mainnet-polygon.brave.com/";

Polygon for DNS resolution #13202

Polygon for DNS resolution #13202

Conversation

supermassive commented Apr 28, 2022 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

kdenhartog left a comment

Choose a reason for hiding this comment

diracdeltas commented May 11, 2022

supermassive commented May 11, 2022

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

goodov left a comment

Choose a reason for hiding this comment

supermassive commented May 12, 2022

goodov May 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdenhartog left a comment • edited Loading

Choose a reason for hiding this comment

supermassive commented Apr 28, 2022 •

edited

Loading

goodov May 12, 2022 •

edited

Loading

kdenhartog left a comment •

edited

Loading