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.

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

Bug removal in resolve #3

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

letmejustputthishere
Copy link
Contributor

@letmejustputthishere letmejustputthishere commented Sep 10, 2020

As parseDomain pushes the domain from left to right onto the list, we would end up with this list for the domain subdomain.dfinity.com (if it would have been implemented correctly, the way it's implemented now the com is missing):

                                           HEAD
                                             +
                                             |
                                             |
+---------+   +---------+  +--------+   +----v---+
| null    +--->subdomain+-->dfinity +--->com     |
|         |   |         |  |        |   |        |
+---------+   +---------+  +--------+   +--------+

In the previous code with List.last<Text>(parsedDomain) we would assign ?subdomain to subdomain inside the first iteration of the while-loop. We would then ask Root-server for subdomain. Instead we should ask root for com which is the head of the list.
Additionally we would have then called List.drop<Text>(parsedDomain, counter) which would have dropped the first 2 elements in our list, leaving us with nothing but

                HEAD
                  +
                  |
                  |
                  v
+---------+   +---+-----+
| null    +--->subdomain|
|         |   |         |
+---------+   +---------+

The proposed changes should fix this.

As `parseDomain` pushes the domain from left to right onto the list, we would end up with this list for the domain `subdomain.dfinity.com` (if it would have been implemented correctly, the way it's implemented now the `com` is missing):

                                           HEAD
                                             +
                                             |
                                             |
+---------+   +---------+  +--------+   +----v---+
| null    +--->subdomain+-->dfinity +--->com     |
|         |   |         |  |        |   |        |
+---------+   +---------+  +--------+   +--------+

In the previous code with `List.last` we would assign `?subdomain` to `subdomain` inside the first iteration of the while-loop. We would then ask Root-`server` for `subdomain`. Instead we should ask root for `com` which is the head of the list. 
Additionally we would have then called `List.drop<Text>(parsedDomain, counter)` which would have dropped the first 2 elements in our list, leaving us with nothing but 

                HEAD
                  +
                  |
                  |
                  v
+---------+   +---+-----+
| null    +--->subdomain|
|         |   |         |
+---------+   +---------+

The proposed changes should fix this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants