chore(docs): ADR for enforcing ArNS undername limits #272
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #272 +/- ##
========================================
Coverage 71.75% 71.75%
========================================
Files 37 37
Lines 9280 9280
Branches 537 537
========================================
Hits 6659 6659
Misses 2617 2617
Partials 4 4 ☔ View full report in Codecov by Sentry. |
dtfiedler
force-pushed
the
PE-7191-arns-undername-enforcement-adr
branch
from
80e02be to
348cb69
Compare
dtfiedler
force-pushed
the
PE-7191-arns-undername-enforcement-adr
branch
from
348cb69 to
4d4e539
Compare
dtfiedler
force-pushed
the
PE-7191-arns-undername-enforcement-adr
branch
from
97dd9fc to
974b07d
Compare
📝 WalkthroughWalkthroughThe document introduces the "ArNS Undername Limit Enforcement" proposal, detailing the enforcement of undername limits for ArNS names as defined by the ARIO network contract. It establishes that increasing these limits necessitates payment in $ARIO tokens to compensate gateway operators for additional computational resources. The proposal mandates that AR.IO gateways enforce these limits during name resolution and addresses the limitations of the Changes
Sequence DiagramsequenceDiagram
participant Client
participant Gateway as AR.IO Gateway
participant ANT as ANT Contract
participant ARIO as ARIO Network Contract
Client->>Gateway: Request ArNS Name Resolution
Gateway->>ANT: Retrieve Name Records
ANT-->>Gateway: Return Records
Gateway->>ARIO: Check Undername Limits
ARIO-->>Gateway: Validate Limits
alt Limits Exceeded
Gateway->>Client: Return 400 Error
else Limits Valid
Gateway->>Client: Return Sorted Records
end
The sequence diagram illustrates the proposed flow for ArNS name resolution, showing how the AR.IO gateway checks undername limits, validates records, and handles potential limit violations during the name resolution process. Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/madr/003-arns-undername-limits.md (2)
10-10: Fix grammatical errors in the context section.The sentence has multiple grammatical issues:
- Missing apostrophe in "name's"
- Run-on sentence structure
-ArNS names have a supported undername limit, defined by the ARIO network contract. Increasing this limit requires payment in $ARIO tokens to compensate gateway operators for the additional computational resources needed to serve undername and promote responsible usage of ArNS. AR.IO gateways must enforce this limit when resolving ArNS names to ensure operators are fairly rewarded for their services through fees paid by increasing a names undername limit and providing a consistent experience across the network. +ArNS names have a supported undername limit, defined by the ARIO network contract. Increasing this limit requires payment in $ARIO tokens to compensate gateway operators for the additional computational resources needed to serve undernames and promote responsible usage of ArNS. AR.IO gateways must enforce this limit when resolving ArNS names to ensure operators are fairly rewarded for their services through fees paid by increasing a name's undername limit and providing a consistent experience across the network.🧰 Tools
🪛 LanguageTool
[uncategorized] ~10-~10: It seems likely that a singular genitive (’s) apostrophe is missing.
Context: ...vices through fees paid by increasing a names undername limit and providing a consist...(AI_HYDRA_LEO_APOSTROPHE_S_XS)
98-98: Fix grammatical issues in Option 2's description.The sentence has several grammatical issues:
-ANTs store additional information in the state for each record, indicating the priority a name. The [ar-io-node] would respect this priority when resolving undernames. If the ANT does not return priority attribute for a, the [ar-io-node] would sort undernames alphabetically. +ANTs store additional information in the state for each record, indicating the priority of a name. The [ar-io-node] would respect this priority when resolving undernames. If the ANT does not return a priority attribute for a record, the [ar-io-node] would sort undernames alphabetically.🧰 Tools
🪛 LanguageTool
[uncategorized] ~98-~98: Possible missing preposition found.
Context: ...or each record, indicating the priority a name. The [ar-io-node] would respect th...(AI_HYDRA_LEO_MISSING_OF)
[uncategorized] ~98-~98: Possible missing article found.
Context: ... undernames. If the ANT does not return priority attribute for a, the [ar-io-node] would...(AI_HYDRA_LEO_MISSING_A)
[grammar] ~98-~98: Articles like ‘a’ are rarely followed by punctuation. A word may be missing after ‘a’, or the punctuation mark may not be necessary.
Context: ... does not return priority attribute for a, the [ar-io-node] would sort undernames ...(THE_PUNCT)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/madr/003-arns-undername-limits.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/madr/003-arns-undername-limits.md
[uncategorized] ~10-~10: It seems likely that a singular genitive (’s) apostrophe is missing.
Context: ...vices through fees paid by increasing a names undername limit and providing a consist...
(AI_HYDRA_LEO_APOSTROPHE_S_XS)
[uncategorized] ~98-~98: Possible missing preposition found.
Context: ...or each record, indicating the priority a name. The [ar-io-node] would respect th...
(AI_HYDRA_LEO_MISSING_OF)
[uncategorized] ~98-~98: Possible missing article found.
Context: ... undernames. If the ANT does not return priority attribute for a, the [ar-io-node] would...
(AI_HYDRA_LEO_MISSING_A)
[grammar] ~98-~98: Articles like ‘a’ are rarely followed by punctuation. A word may be missing after ‘a’, or the punctuation mark may not be necessary.
Context: ... does not return priority attribute for a, the [ar-io-node] would sort undernames ...
(THE_PUNCT)
[grammar] ~170-~170: Did you mean “implementing”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ...esponse type is unchanged) - + Allows to implement Option #1 at a later time, if desired -...
(ALLOW_TO)
[formatting] ~185-~185: Consider inserting a comma before ‘however’.
Context: ...contract, and the [ar-io-node]. Option #1 however, requires changes suggested in Option #2...
(HOWEVER_MISSING_COMMA)
[uncategorized] ~187-~187: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ect type. Option #2 requires the least amount of changes, while satisfying the requir...
(AMOUNTOF_TO_NUMBEROF)
🔇 Additional comments (4)
docs/madr/003-arns-undername-limits.md (4)
20-39: LGTM! Clear and comprehensive sequence diagram.The sequence diagram effectively illustrates the flow of ArNS name resolution and undername limit enforcement, including error handling.
214-214: Address the TODO comment about pros/cons.The pros and cons section for each option needs to be completed.
1-228: Overall, this is a well-structured and comprehensive ADR.The document effectively communicates the architectural decision for ArNS undername limit enforcement. The technical content is accurate, and the sequence diagram clearly illustrates the flow. While there are some minor grammatical issues to address, the core content and decision-making process are well documented.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~10-~10: It seems likely that a singular genitive (’s) apostrophe is missing.
Context: ...vices through fees paid by increasing a names undername limit and providing a consist...(AI_HYDRA_LEO_APOSTROPHE_S_XS)
[uncategorized] ~98-~98: Possible missing preposition found.
Context: ...or each record, indicating the priority a name. The [ar-io-node] would respect th...(AI_HYDRA_LEO_MISSING_OF)
[uncategorized] ~98-~98: Possible missing article found.
Context: ... undernames. If the ANT does not return priority attribute for a, the [ar-io-node] would...(AI_HYDRA_LEO_MISSING_A)
[grammar] ~98-~98: Articles like ‘a’ are rarely followed by punctuation. A word may be missing after ‘a’, or the punctuation mark may not be necessary.
Context: ... does not return priority attribute for a, the [ar-io-node] would sort undernames ...(THE_PUNCT)
[grammar] ~170-~170: Did you mean “implementing”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ...esponse type is unchanged) -+Allows to implement Option #1 at a later time, if desired -...(ALLOW_TO)
[formatting] ~185-~185: Consider inserting a comma before ‘however’.
Context: ...contract, and the [ar-io-node]. Option #1 however, requires changes suggested in Option #2...(HOWEVER_MISSING_COMMA)
[uncategorized] ~187-~187: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ect type. Option #2 requires the least amount of changes, while satisfying the requir...(AMOUNTOF_TO_NUMBEROF)
213-213: Verify the cache timing document reference.The document references
[./002-arns-cache-timing.md]for caching logic. Let's verify this document exists and contains the relevant information.✅ Verification successful
Reference to cache timing document is accurate and valid
The document
002-arns-cache-timing.mdexists and contains comprehensive caching logic, TTL configurations, and cache update mechanisms that align with the gateway requirements mentioned.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and content of the referenced cache timing document # Check if the file exists if [ -f "docs/madr/002-arns-cache-timing.md" ]; then echo "Cache timing document exists" # Search for caching-related content rg -i "cache|ttl" "docs/madr/002-arns-cache-timing.md" else echo "Cache timing document not found" fiLength of output: 1612
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
docs/madr/003-arns-undername-limits.md (3)
98-98: Fix grammatical error in Option 2 description.There's a missing word in the sentence.
-ANTs store additional information in the state for each record, indicating the priority a name. +ANTs store additional information in the state for each record, indicating the priority of a name.🧰 Tools
🪛 LanguageTool
[grammar] ~98-~98: Articles like ‘a’ are rarely followed by punctuation. A word may be missing after ‘a’, or the punctuation mark may not be necessary.
Context: ... does not return priority attribute for a, the [ar-io-node] would sort undernames ...(THE_PUNCT)
215-221: Enhance the next steps section.The next steps would benefit from:
- Adding pros/cons for each step as mentioned in the previous comment
- Fixing the duplicate word in the SDK API return type description
-Modify ar-io-sdk `getRecords()` API return type to return include `priority` as as `number | undefined` +Modify ar-io-sdk `getRecords()` API return type to include `priority` as `number | undefined`🧰 Tools
🪛 LanguageTool
[duplication] ~218-~218: Possible typo: you repeated a word.
Context: ...eturn type to return includepriorityas asnumber | undefined- Update `ar-io-no...(ENGLISH_WORD_REPEAT_RULE)
211-212: Consider clarifying error handling behavior.The error handling description could be more specific about what constitutes an "invalid priority order" and provide examples.
Consider adding examples of invalid priority scenarios, such as:
{ "@": { "priority": 0 }, "undername1": { "priority": 1 }, "undername2": { "priority": 1 } // Conflicting priority }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/madr/003-arns-undername-limits.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/madr/003-arns-undername-limits.md
[grammar] ~98-~98: Articles like ‘a’ are rarely followed by punctuation. A word may be missing after ‘a’, or the punctuation mark may not be necessary.
Context: ... does not return priority attribute for a, the [ar-io-node] would sort undernames ...
(THE_PUNCT)
[grammar] ~170-~170: Did you mean “implementing”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ...esponse type is unchanged) - + Allows to implement Option #1 at a later time, if desired -...
(ALLOW_TO)
[formatting] ~185-~185: Consider inserting a comma before ‘however’.
Context: ...contract, and the [ar-io-node]. Option #1 however, requires changes suggested in Option #2...
(HOWEVER_MISSING_COMMA)
[uncategorized] ~187-~187: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ect type. Option #2 requires the least amount of changes, while satisfying the requir...
(AMOUNTOF_TO_NUMBEROF)
[duplication] ~218-~218: Possible typo: you repeated a word.
Context: ...eturn type to return include priority as as number | undefined - Update `ar-io-no...
(ENGLISH_WORD_REPEAT_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test (macos-latest)
- GitHub Check: test (ubuntu-latest)
🔇 Additional comments (2)
docs/madr/003-arns-undername-limits.md (2)
10-10: Fix grammatical issues in the context.The sentence structure needs improvement for better clarity.
-ArNS names have a supported undername limit, defined by the ARIO network contract. Increasing this limit requires payment in $ARIO tokens to compensate gateway operators for the additional computational resources needed to serve undername and promote responsible usage of ArNS. AR.IO gateways must enforce this limit when resolving ArNS names to ensure operators are fairly rewarded for their services through fees paid by increasing a names undername limit and providing a consistent experience across the network. +ArNS names have a supported undername limit, defined by the ARIO network contract. Increasing a name's limit requires payment in $ARIO tokens to compensate gateway operators for the additional computational resources needed to serve undernames and promote responsible usage of ArNS. AR.IO gateways must enforce this limit when resolving ArNS names to ensure operators are fairly rewarded for their services through fees paid by increasing undername limits and providing a consistent experience across the network.
20-39: LGTM! Well-documented sequence flow.The sequence diagram effectively illustrates the ArNS name resolution process and undername limit enforcement.
dtfiedler
force-pushed
the
PE-7191-arns-undername-enforcement-adr
branch
from
49f8430 to
61d298f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
docs/madr/003-arns-undername-limits.md (6)
5-5: Update the date to the current date.The document is dated in the future (2025-01-14). Please update it to the current date.
98-98: Fix grammatical issues for better clarity.Consider these corrections:
- "indicating the priority of a name"
- "does not return a priority attribute"
🧰 Tools
🪛 LanguageTool
[uncategorized] ~98-~98: Possible missing preposition found.
Context: ...or each record, indicating the priority a name. The [ar-io-node] would respect th...(AI_HYDRA_LEO_MISSING_OF)
[uncategorized] ~98-~98: Possible missing article found.
Context: ... undernames. If the ANT does not return priority attribute for a record, the [ar-io-node...(AI_HYDRA_LEO_MISSING_A)
20-39: Consider adding more error scenarios to the sequence diagram.The diagram effectively shows the "Payment required" error case, but consider adding other potential error scenarios:
- Invalid priority order (400 error)
- ANT contract not found
- Network timeouts
102-112: Enhance error handling in the TypeScript example.Consider adding error handling for edge cases:
const records = getRecords(name); const sortedRecords = Object.entries(records).sort(([a], [b]) => { try { if ('priority' in a && 'priority' in b) { // Validate priority values are numbers if (typeof a.priority !== 'number' || typeof b.priority !== 'number') { throw new Error('Invalid priority type'); } return a.priority - b.priority; } return a.localeCompare(b); } catch (error) { throw new Error(`Failed to sort records: ${error.message}`); } });
185-185: Fix grammar: Add comma before 'however'.Change to: "Option #1, however, requires changes..."
🧰 Tools
187-187: Improve wording: 'amount of' -> 'number of'.Change "least amount of changes" to "least number of changes" as we're referring to countable changes.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~187-~187: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ect type. Option #2 requires the least amount of changes, while satisfying the requir...(AMOUNTOF_TO_NUMBEROF)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/madr/003-arns-undername-limits.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/madr/003-arns-undername-limits.md
[uncategorized] ~98-~98: Possible missing preposition found.
Context: ...or each record, indicating the priority a name. The [ar-io-node] would respect th...
(AI_HYDRA_LEO_MISSING_OF)
[uncategorized] ~98-~98: Possible missing article found.
Context: ... undernames. If the ANT does not return priority attribute for a record, the [ar-io-node...
(AI_HYDRA_LEO_MISSING_A)
[formatting] ~185-~185: Consider inserting a comma before ‘however’.
Context: ...contract, and the [ar-io-node]. Option #1 however, requires changes suggested in Option #2...
(HOWEVER_MISSING_COMMA)
[uncategorized] ~187-~187: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ect type. Option #2 requires the least amount of changes, while satisfying the requir...
(AMOUNTOF_TO_NUMBEROF)
🔇 Additional comments (1)
docs/madr/003-arns-undername-limits.md (1)
217-220: Add pros/cons for each next step.As mentioned in the previous review, please add pros/cons for each next step to help readers understand the implications and trade-offs of each task.
| } | ||
| ``` | ||
|
|
||
| The [ar-io-node] will sort the records by priority, and fallback to alphabetical sorting when the priority attribute is not present and enforce the undername limit against the sorted records. If ANTs provide invalid priority order (conflicting records with same priority), **the [ar-io-node] will return a 400 error to notify the ANT owner of invalid priority order.** |
There was a problem hiding this comment.
It might be better to define a sort order (e.g., priority, alphabetical) that can be applied when there are conflicting priorities instead of returning an error.
There was a problem hiding this comment.
specified that operators can specify how they want to sort these scenarios - and default with be lexical 90be780
|
see no issues with this path forward and I think it solves the issues outlined. |
|
|
||
| ArNS names have a supported undername limit, defined by the ARIO network contract. Increasing this limit requires payment in $ARIO tokens to compensate gateway operators for the additional computational resources needed to serve undername and promote responsible usage of ArNS. AR.IO gateways must enforce this limit when resolving ArNS names to ensure operators are fairly rewarded for their services through fees paid by increasing a name's undername limit and providing a consistent experience across the network. | ||
|
|
||
| Currently, the `getRecords` API on ANTs return **a table of records** allowing for efficient undername lookups. Lua does not guarantee the order of keys in a table, making it difficult to know what undernames to enforce. Sorting needs to either 1. be done by the ANT and provided as an array of records, or 2. be done by the [ar-io-node] using additional information provided by the ANT. Additionally, caching within the ar-io-node needs to properly handle these priority order changes. |
There was a problem hiding this comment.
API on ANTs return -> API on ANTs returns
There was a problem hiding this comment.
The other alternative is that the ANT does the sorting and still returns the map but capped at the appropriate size. However if the ANT does not comply with that responsibility, the node is left to make enforcement decisions on its own again (i.e. back to square one problem).
There was a problem hiding this comment.
this would requrie the ANT to check the contract for the limit, which requires a coroutine
|
|
||
| - Honor ANT priority ordering when available | ||
| - Consistent enforcement of undername limits across AR.IO gateways | ||
| - Simple fallback mechanism if ANT does not return or contain priority data |
There was a problem hiding this comment.
This is a wacky scenario, but what if an ANT doesn't consistently return its map and also doesn't provide prioritization ordering such that different gateways might end up with different valid undernames? Obviously this primarily hurts the users of the undernames, but I'm trying to decide whether this negatively impacts gateways in a way that hurts the protocol. Since the observers don't yet check undernames I think we'd be ok?
There was a problem hiding this comment.
I think that's something worth pointing out in this ADR: ANTs should be considered untrustworthy 🤔
There was a problem hiding this comment.
good call out - will add
|
|
||
| ### Option 2: ANTS provide priority data in records | ||
|
|
||
| ANTs store additional information in the state for each record, indicating the priority a name. The [ar-io-node] would respect this priority when resolving undernames. If the ANT does not return priority attribute for a record, the [ar-io-node] would sort undernames alphabetically. |
There was a problem hiding this comment.
sort undernames alphabetically
I think that's fine in theory. But just be mindful that this means different things in different use cases. e.g. lexical vs. numeric sorting of numbers.
There was a problem hiding this comment.
specified they will default lexical sorting
| return a.localeCompare(b); | ||
| }); | ||
|
|
||
| // enforce undername limit against sorted records, using the priority field, fallback to alphabetical |
There was a problem hiding this comment.
So how would we handle cases where:
- Some undernames have priority ordering data but others don't
- ... and in the above case, there are N that are priority ordered and M where they arent where...
- ... N < max undernames for the records
- ... N > max undernames for the records
- ... N == max undernames for the records
There was a problem hiding this comment.
good call out - i'll add a comment that priority will always be respected over lack of the attribute. i.e. all records with priority will be served before the array of names without get potentially served.
There was a problem hiding this comment.
added specification here - 90be780
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
docs/madr/003-arns-undername-limits.md (8)
14-14: Stylistic Improvement Suggestion:
Consider replacing “can not” with “cannot” and rephrasing “due to the fact that” to “because” to improve readability.🧰 Tools
🪛 LanguageTool
[style] ~14-~14: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ... It's also important to note that ANTs can not generally be trusted to return the same...(CAN_NOT_PREMIUM)
[style] ~14-~14: ‘due to the fact that’ might be wordy. Consider a shorter alternative.
Context: ...n the same values all the time. This is due to the fact that ANTs are not deployed as part of the AR...(EN_WORDINESS_PREMIUM_DUE_TO_THE_FACT_THAT)
58-73: Lua Code Sample for Option 1:
The example shows how the existinggetRecordsfunction is structured and suggests the new sorted format. It would be helpful to add a note clarifying that this snippet is pseudo-code and to verify standard Lua syntax (e.g., using curly braces{}rather than parentheses for table definitions).
98-100: Option 2 Introduction:
The description introduces embedding a priority attribute within ANT records nicely. For clarity, consider inserting an article: “if the ANT does not return a priority attribute for a record…”🧰 Tools
🪛 LanguageTool
[uncategorized] ~100-~100: Possible missing article found.
Context: ... undernames. If the ANT does not return priority attribute for a record, the [ar-io-node...(AI_HYDRA_LEO_MISSING_A)
131-145: Lua Example for Option 3:
The Lua snippet for defining global sort attributes is illustrative. Similar to earlier Lua blocks, consider confirming that the pseudo-code follows standard Lua conventions, and annotate as necessary that this is an example.
168-174: Pros and Cons for Option 1:
The bullet list lays out the trade-offs clearly. In light of earlier feedback (e.g., dtfiedler's TODO), you might consider adding further pros/cons to deepen the analysis.
190-197: Decision Section Clarity:
The decision to implement Option #2 is well justified. For improved phrasing, consider replacing “least amount of changes” with “fewest changes” to better align with standard wording.🧰 Tools
🪛 LanguageTool
[uncategorized] ~196-~196: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ect type. Option #2 requires the least amount of changes, while satisfying the requir...(AMOUNTOF_TO_NUMBEROF)
225-225: Explanation of Fallback Sorting:
The sentence describing how the [ar-io-node] falls back to lexicographical sorting when a priority is missing could be streamlined. For example, consider: “...fallback to lexicographical sorting for records that do not have a defined priority.”
229-235: Next Steps Clarity:
The actionable items clearly outline what needs to be updated across the ANT source, SDK, node, and app. On line 233, the phrase “to return include” appears grammatically awkward. It could be rephrased to: “Modify [ar-io-sdk]getRecords()API return type to includepriorityasnumber | undefined.”
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/madr/003-arns-undername-limits.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/madr/003-arns-undername-limits.md
[style] ~14-~14: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ... It's also important to note that ANTs can not generally be trusted to return the same...
(CAN_NOT_PREMIUM)
[style] ~14-~14: ‘due to the fact that’ might be wordy. Consider a shorter alternative.
Context: ...n the same values all the time. This is due to the fact that ANTs are not deployed as part of the AR...
(EN_WORDINESS_PREMIUM_DUE_TO_THE_FACT_THAT)
[uncategorized] ~100-~100: Possible missing article found.
Context: ... undernames. If the ANT does not return priority attribute for a record, the [ar-io-node...
(AI_HYDRA_LEO_MISSING_A)
[uncategorized] ~196-~196: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ect type. Option #2 requires the least amount of changes, while satisfying the requir...
(AMOUNTOF_TO_NUMBEROF)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test (macos-latest)
🔇 Additional comments (12)
docs/madr/003-arns-undername-limits.md (12)
1-7: Header & Metadata Verification:
The document header clearly states "ArNS Undername Limit Enforcement" and provides essential metadata (status, deciders, date, authors). Everything looks in order.
10-12: Context and Problem Statement Clarity:
The background section explains how undername limits, associated fees (in $ARIO tokens), and the enforcement challenges are related. The technical context is well communicated.
16-21: Priority Resolution Flow & Diagram:
The “Priority Resolution Flow” heading followed by the explanatory text and the Mermaid diagram effectively visualize the name resolution process. Ensure that this diagram is kept up-to-date with any future logic changes.
43-48: Decision Drivers Overview:
The bullet list succinctly captures the key drivers—such as honoring ANT priority ordering and consistent enforcement of limits. The inclusion of fallback mechanisms is clearly emphasized.
50-54: Options Overview for ANT Sorting:
The description of Option 1 explains the issue with Lua tables and outlines the idea of returning a sorted array. The rationale is clear.
75-95: Lua Sorting Logic in Option 1:
ThegetSortedRecordsfunction demonstrates a viable approach to sorting. A couple of points to verify:
- Confirm that the comparator (
return a.record > b.record) produces the intended lexicographical order (typically ascending order is desired).- The special handling for the
"@"record is good; just ensure this behavior is consistent with overall design expectations.
96-96: Explanation of Sorted Records:
The statement clarifies that the sorted array is what the [ar-io-node] will enforce. This is clear enough, though adding a brief note on any assumptions regarding the array structure could further enhance clarity.
123-125: Option 3 Overview:
The introduction to Option 3 is concise. It clearly sets up the concept of providing sort attributes via a separate API.
175-182: Pros and Cons for Option 2:
The benefits and drawbacks are summarized effectively. It might be useful to elaborate on any potential performance considerations due to sorting on the node side.
184-188: Pros and Cons for Option 3:
This section mirrors Option 2’s points, noting the trade-off of additional API calls. It is succinct and clear.
227-228: Caching Mechanism Explanation:
The reference to the caching logic defined in [./002-arns-cache-timing.md] is a valuable detail that connects this ADR to other parts of the system. The explanation is clear and complete.
237-254: Reference Links and Resources:
The list of links and references is comprehensive and valuable for readers seeking additional context. All resources appear relevant and current.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
docs/madr/003-arns-undername-limits.md (5)
8-14: Enhance Language Clarity in the Problem Statement
The "Context and Problem Statement" section is comprehensive. A couple of minor language tweaks are recommended for clarity and style:
- Replace “can not” with “cannot” for consistency.
- Consider replacing “due to the fact that” with “because” to reduce wordiness.
These adjustments will help tighten the prose without altering meaning.🧰 Tools
🪛 LanguageTool
[style] ~14-~14: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ... It's also important to note that ANTs can not generally be trusted to return the same...(CAN_NOT_PREMIUM)
[style] ~14-~14: ‘due to the fact that’ might be wordy. Consider a shorter alternative.
Context: ...n the same values all the time. This is due to the fact that ANTs are not deployed as part of the AR...(EN_WORDINESS_PREMIUM_DUE_TO_THE_FACT_THAT)
43-47: Decision Drivers Section – Consider Clarifying Terminology
The decision drivers are well listed. However, the phrase “respects existing ANT records object type” could use a brief clarification so that readers unfamiliar with the terminology understand its significance.
98-101: Option 2: Explanation of Handling Missing Priority Data
The description for Option 2 is concise and clear. It might be useful to elaborate (even briefly) on how records without the priority attribute are handled beyond stating that they will be sorted lexicographically.🧰 Tools
🪛 LanguageTool
[uncategorized] ~100-~100: Possible missing article found.
Context: ... undernames. If the ANT does not return priority attribute for a record, the [ar-io-node...(AI_HYDRA_LEO_MISSING_A)
148-171: Pros and Cons Section – Minor Wording Suggestion
The pros and cons for the three options are outlined comprehensively. For clarity, consider changing “requires the least amount of changes” (line ~178) to “requires the least number of changes” to reflect countable modifications.
211-218: Next Steps Outline Actionable Items
The “Next steps” section lists clear follow-up actions. For improved tracking, you might consider adding target dates or responsible owners for each task. This could facilitate accountability during implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/madr/003-arns-undername-limits.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/madr/003-arns-undername-limits.md
[style] ~14-~14: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ... It's also important to note that ANTs can not generally be trusted to return the same...
(CAN_NOT_PREMIUM)
[style] ~14-~14: ‘due to the fact that’ might be wordy. Consider a shorter alternative.
Context: ...n the same values all the time. This is due to the fact that ANTs are not deployed as part of the AR...
(EN_WORDINESS_PREMIUM_DUE_TO_THE_FACT_THAT)
[uncategorized] ~100-~100: Possible missing article found.
Context: ... undernames. If the ANT does not return priority attribute for a record, the [ar-io-node...
(AI_HYDRA_LEO_MISSING_A)
[uncategorized] ~178-~178: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ect type. Option #2 requires the least amount of changes, while satisfying the requir...
(AMOUNTOF_TO_NUMBEROF)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test (macos-latest)
- GitHub Check: test (ubuntu-latest)
🔇 Additional comments (6)
docs/madr/003-arns-undername-limits.md (6)
1-7: Header Information is Clear and Informative
The title and header section (Status, Deciders, Date, Authors) succinctly introduce the ADR. Consider adding a brief explanation of each decider’s role if you believe additional context would benefit future readers.
22-41: Sequence Diagram Effectively Illustrates the Flow
The included Mermaid sequence diagram clearly outlines the priority resolution flow between the Client, AR.IO Gateway, ARIOContract, and ANTContract. For even greater clarity, you might consider labeling how TTL and caching are handled in the flow.
74-93: Review of Lua Sorting Logic in Option 1
The Lua snippet that defines a custom sorting function is clear, particularly the handling to always place the "@" record first. One point to verify: the comparator usesreturn a.record > b.record(line 90) to order the remaining items. Ensure this correctly implements the intended lexicographical order (it may be that<is desired for ascending order). A brief comment explaining the chosen operator would improve maintainability.
131-143: TypeScript Sorting Example – Destructuring Caution
The provided TypeScript example for sorting ANT records uses.sort(([a], [b]) => { ... }). Note that this destructuring returns only the keys rather than the key–value pairs. To ensure proper access to the sort attributes (e.g.,recordA[sortKey]), consider destructuring as follows:- Object.entries(records).sort(([a], [b]) => { ... }) + Object.entries(records).sort(([keyA, recordA], [keyB, recordB]) => { + if ('priority' in recordA && 'priority' in recordB) { + return sortOrder === 'desc' + ? recordA[sortKey] - recordB[sortKey] + : recordB[sortKey] - recordA[sortKey]; + } + // fallback to lexicographical sorting on keys + return keyA.localeCompare(keyB); + });This recommendation echoes earlier feedback on similar examples.
172-179: Decision Section Clearly States the Chosen Approach
The ADR clearly commits to implementing Option #2 and explains the rationale behind the decision. This clarity is beneficial for maintainability. Ensure that future updates to the ANT records contract remain backward-compatible with existing systems.🧰 Tools
🪛 LanguageTool
[uncategorized] ~178-~178: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ect type. Option #2 requires the least amount of changes, while satisfying the requir...(AMOUNTOF_TO_NUMBEROF)
219-236: Links Section is Comprehensive and Up-to-Date
The provided links to related documentation and external resources are thorough. It may be useful to periodically verify that these external references remain current as the ecosystem evolves.
No description provided.