Skip to content

docs(builder): clarify Felt division across builder docs#284

Open
alimmaster wants to merge 4 commits into
0xMiden:mainfrom
alimmaster:patch-1
Open

docs(builder): clarify Felt division across builder docs#284
alimmaster wants to merge 4 commits into
0xMiden:mainfrom
alimmaster:patch-1

Conversation

@alimmaster
Copy link
Copy Markdown

Current behavior

The builder docs mention that Felt division uses the multiplicative inverse, but the explanation is fragmented and easy to miss.

As a result, developers can misread Felt division as standard integer division, especially in business-logic scenarios like token amounts, fee calculations, and proportional splits.

New behavior

This change improves Felt division guidance in three places:

  • adds a dedicated Division subsection to types.md with side-by-side u64 vs Felt examples
  • adds a When to use Felt vs u64 section to patterns.md
  • adds a dedicated Felt division pitfall entry to pitfalls.md

Breaking changes

None.

Other info

Documentation-only update.
Closes #232.

Copy link
Copy Markdown
Contributor

@BrianSeong99 BrianSeong99 left a comment

Choose a reason for hiding this comment

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

Thanks for this — the core is solid: the math checks out (20/3 → 6148914689804861447, 10/2 → 5), the field-division explanation is accurate, and the u64 vs Felt guidance is genuinely useful. A few changes before merge:

1. Bug — Word has no inner field (pitfalls.md)

In the Solution snippet:

let total_amount: u64 = total.inner[0].as_u64();

Word is accessed via its .a/.b/.c/.d fields (or Index), not .inner — every other Word example in the docs uses field syntax. This won't compile as written. Please change to:

let total_amount: u64 = total.a.as_u64();

2. Trim the over-warning (pitfalls.md)

The entry is a bit heavy: Problem → "especially dangerous" → Why → Solution → plus a :::warning box that largely restates the Solution. Suggest:

  • Drop the :::warning Do not use Felt division… callout — Problem/Why/Solution already covers it, and it duplicates the paragraph right above it.
  • Soften "especially dangerous" to something like "a common source of bugs." It's well-defined arithmetic, just not what integer-minded code expects.

3. Optional polish

  • Keep the divide example consistent: types.md uses felt!(20) / felt!(3), pitfalls.md uses Felt::new(20) / Felt::new(3). Prefer felt!() in both.
  • "as_u64() … is zero-cost" (patterns.md) — "inexpensive" is safer; the conversion can involve a canonical reduction.

With #1 fixed and #2 trimmed this is good to merge. Nice addition overall.

@alimmaster
Copy link
Copy Markdown
Author

Addressed the review feedback: fixed the Word access example, trimmed the duplicate warning callout, softened the wording, aligned the Felt division examples on felt!(), and changed zero-cost to inexpensive.

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.

Improve Felt division and field arithmetic documentation across builder docs

2 participants