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

[ADR] 0001 - Adopt Multiaddr #32

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

[ADR] 0001 - Adopt Multiaddr #32

wants to merge 6 commits into from

Conversation

bryanchriswhite
Copy link
Contributor

@bryanchriswhite bryanchriswhite commented Apr 14, 2023

Summary

This pull request adds an Architecture Decision Record (ADR) for adopting the multiaddr addressing format to simplify and consolidate node identity in our system. The ADR explains the context, problem statement, decision drivers, considered options, and the decision outcome, along with the positive and negative consequences of the chosen option.

The chosen option is to adopt the multiaddr format as it provides a standardized, extensible, and future-proof solution for addressing nodes in our network.

Please review the ADR and provide feedback on:

  • The clarity of the problem statement and context
  • The thoroughness of the decision drivers and considered options
  • The appropriateness of the chosen option given the decision drivers
  • Any additional considerations that may have been overlooked
  • Any changes in formatting, grammar, or language for better readability

@bryanchriswhite bryanchriswhite added the architecture decision Related to architecture decision records (ADRs) label Apr 14, 2023
@bryanchriswhite bryanchriswhite self-assigned this Apr 14, 2023
@bryanchriswhite bryanchriswhite added p2p consensus Consensus specific changes and removed consensus Consensus specific changes labels Apr 14, 2023
@bryanchriswhite bryanchriswhite changed the base branch from main to docs/ADRs April 14, 2023 11:46
@bryanchriswhite bryanchriswhite force-pushed the docs/ADRs branch 2 times, most recently from 8e332c4 to 7e6934a Compare April 14, 2023 12:47
@bryanchriswhite bryanchriswhite changed the base branch from docs/ADRs to main April 17, 2023 10:56
@bryanchriswhite bryanchriswhite marked this pull request as ready for review April 17, 2023 10:59
@bryanchriswhite bryanchriswhite changed the title [ADR] 0001 - Adopting Multiaddr for Addressing and Protocol Handling [ADR] 0001 - Adopt Multiaddr Apr 17, 2023
@bryanchriswhite bryanchriswhite marked this pull request as draft April 18, 2023 17:34
@Olshansk Olshansk removed their request for review April 19, 2023 19:43
@bryanchriswhite bryanchriswhite marked this pull request as ready for review April 21, 2023 07:13
Copy link
Contributor

@dylanlott dylanlott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

multiaddr is the right choice here I think. It's a well-defined and supported format and it doesn't make sense for us to spend time reinventing the wheel.

ADRs/0001-adopt-multiaddr.md Outdated Show resolved Hide resolved
ADRs/0001-adopt-multiaddr.md Outdated Show resolved Hide resolved
@@ -0,0 +1,60 @@
# ADR-0001: Adopt Multiaddr
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

ADRs/0001-adopt-multiaddr.md Outdated Show resolved Hide resolved
ADRs/0001-adopt-multiaddr.md Outdated Show resolved Hide resolved
ADRs/0001-adopt-multiaddr.md Outdated Show resolved Hide resolved
* Good, because it doesn't require an additional dependency
* Bad, because it may lead to a non-standard and less extensible solution
* Bad, because it requires development and maintenance efforts

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that the ADR is very light, which comes with both pros & cons.

Was it an explicit decision to not, for example, provide examples of what a multiaddr or a custom URL looks like?

I can steelman both sides of the argument here but wanted to know if it was the intention or just a lack of time?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I think it was more of a matter of structure.

Looking at it now, I'm not sure where it would make the most sense to put something like that. I feel like that would be something that I would expect to be covered by references. However, there isn't a great way to consistently be much more specific with a reference other than a link to a page (and maybe a named anchor on the page). I wonder if grouping them all together is the must useful approach.

We could consider including an "examples" or "practical application" section in "decision outcome" > "positive(/negative) consequences" and/or each option section. Pos./neg. consequences would provide a place to show (or reference) code and/or diagram(s) like this. Wdyt?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a section like so with the comments I have in place. I think we should keep it light weight but have a reminder to do this if we choose to.

I think if you copy-paste one of your tests in p2p, or link to them, it would be more than sufficient in this ADR.

### Supporting Examples/References <!-- optional -->
<!-- 
This is an optional section for you to link to or embed supporting examples.
These could come in the form of positive/negative consequences or just
examples for others to reference.
-->

@Olshansk
Copy link
Member

Olshansk commented May 16, 2023

@bryanchriswhite Bump. Wanted to tend to the comments above but otherwise approve the ADR.

* pokt/main:
  styling
  updates contribution process in ADR README
  update to unsuitable terminology
  Removes conclusion from problem statement section
  adds details to README about withdrawing ADRs
  [ADRs] Update ADR docs & template (#36)
  ADR Template Updates (#40)
@bryanchriswhite bryanchriswhite force-pushed the docs/ADR0001 branch 2 times, most recently from 13a0a27 to 8758154 Compare May 19, 2023 13:18
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a few minor comments, but approving because agreed on the path forward.

ADRs/0001-adopt-multiaddr.md Show resolved Hide resolved
ADRs/0001-adopt-multiaddr.md Outdated Show resolved Hide resolved
ADRs/0001-adopt-multiaddr.md Show resolved Hide resolved
Co-authored-by: Daniel Olshansky <[email protected]>
@Olshansk
Copy link
Member

@bryanchriswhite You think this one is good to merge in?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
architecture decision Related to architecture decision records (ADRs) p2p
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

3 participants