-
Notifications
You must be signed in to change notification settings - Fork 59
docs: document the SEP-38 fee-denomination identity #404
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,113 @@ | ||||||||||||||||
| # SEP-38 fee-denomination identity | ||||||||||||||||
|
|
||||||||||||||||
| SEP-38 is how a Stellar anchor answers "what will you give me for this?" — | ||||||||||||||||
| sell USDC, buy naira, with a fee. The fee is where the trap lives. | ||||||||||||||||
|
|
||||||||||||||||
| --- | ||||||||||||||||
|
|
||||||||||||||||
| ## The problem | ||||||||||||||||
|
|
||||||||||||||||
| SEP-38 returns a fee that may be denominated in **either** the sell asset or | ||||||||||||||||
| the buy asset. The response tells you which via `fee.asset`. This is easy to | ||||||||||||||||
| miss, and getting it wrong produces a **unit error** rather than a crash — the | ||||||||||||||||
| arithmetic succeeds and the number is simply wrong. | ||||||||||||||||
|
|
||||||||||||||||
| ### A worked example of the bug | ||||||||||||||||
|
|
||||||||||||||||
| The spec's own worked example is exactly the shape a remittance uses: | ||||||||||||||||
|
|
||||||||||||||||
| ``` | ||||||||||||||||
| sell 100 USDC → buy 500 BRL, price 0.18, fee 10.00 in USDC | ||||||||||||||||
| ``` | ||||||||||||||||
|
|
||||||||||||||||
| Naively computing a pre-fee gross as `buy_amount + fee`: | ||||||||||||||||
|
|
||||||||||||||||
| ``` | ||||||||||||||||
| 500 BRL + 10 USDC = 510 ??? | ||||||||||||||||
| ``` | ||||||||||||||||
|
|
||||||||||||||||
| Ten units of USDC added to five hundred units of BRL — a meaningless quantity | ||||||||||||||||
| that looks plausible enough to ship. No error is raised. The published figure | ||||||||||||||||
| is wrong. | ||||||||||||||||
|
|
||||||||||||||||
| --- | ||||||||||||||||
|
|
||||||||||||||||
| ## The identity | ||||||||||||||||
|
|
||||||||||||||||
| The spec gives two definitions of price depending on where the fee sits: | ||||||||||||||||
|
|
||||||||||||||||
| ``` | ||||||||||||||||
| fee in sell asset: price = (sell_amount - fee) / buy_amount | ||||||||||||||||
| fee in buy asset: price = sell_amount / (buy_amount + fee) | ||||||||||||||||
| ``` | ||||||||||||||||
|
|
||||||||||||||||
| Solving each for the pre-fee gross, expressed in buy-asset units, gives the | ||||||||||||||||
| **same expression** in both cases: | ||||||||||||||||
|
|
||||||||||||||||
| ``` | ||||||||||||||||
| gross_in_buy_asset = sell_amount / price | ||||||||||||||||
| ``` | ||||||||||||||||
|
|
||||||||||||||||
| This is the identity. It needs no branch on `fee.asset`, and it is correct | ||||||||||||||||
| whichever denomination the anchor chose. | ||||||||||||||||
|
|
||||||||||||||||
| ### Verification against the spec example | ||||||||||||||||
|
|
||||||||||||||||
| With a sell-asset fee of 10 USDC: | ||||||||||||||||
|
|
||||||||||||||||
| ``` | ||||||||||||||||
| gross = 100 / 0.18 = 555.56 BRL | ||||||||||||||||
| fee = 555.56 - 500 = 55.56 BRL (not 10 USDC) | ||||||||||||||||
| ``` | ||||||||||||||||
|
|
||||||||||||||||
| The fee the user sees is 55.56 BRL — five times the raw `fee.total` — because | ||||||||||||||||
| the anchor denominated its fee in the sell asset while the user counts in the | ||||||||||||||||
| buy asset. Converting the fee into the recipient's currency is the correct | ||||||||||||||||
| thing to show; showing 10 USDC alongside 500 BRL would be mixing units. | ||||||||||||||||
|
|
||||||||||||||||
| --- | ||||||||||||||||
|
|
||||||||||||||||
| ## How the implementation uses it | ||||||||||||||||
|
|
||||||||||||||||
| `sep38.Quote.normalize()` computes: | ||||||||||||||||
|
|
||||||||||||||||
| ```go | ||||||||||||||||
| q.GrossBuyAmount = q.SellAmount.Div(q.Price) | ||||||||||||||||
| q.FeeInBuyAsset = q.GrossBuyAmount.Sub(q.BuyAmount) | ||||||||||||||||
| ``` | ||||||||||||||||
|
|
||||||||||||||||
| No branch. No switch on `fee.asset`. The same two lines handle both | ||||||||||||||||
| denominations because the identity is the same in both cases. | ||||||||||||||||
|
|
||||||||||||||||
| `FeeInBuyAsset` is the figure shown to users: the fee expressed in the | ||||||||||||||||
| currency the recipient is counting, whatever denomination the anchor used. | ||||||||||||||||
|
|
||||||||||||||||
| --- | ||||||||||||||||
|
|
||||||||||||||||
| ## Why the golden tests exist | ||||||||||||||||
|
|
||||||||||||||||
| A regression in this arithmetic would not crash. It would quietly change a | ||||||||||||||||
| published figure — a worse failure mode than a panic, because it ships. | ||||||||||||||||
|
|
||||||||||||||||
| The golden files in `sep38/testdata/golden/` pin the expected output on disk | ||||||||||||||||
| rather than in an assertion that somebody could adjust while "fixing" a failing | ||||||||||||||||
| test. Three cases are pinned: | ||||||||||||||||
|
Comment on lines
+92
to
+94
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Describe golden-file protection accurately. The supplied Proposed wording-The golden files in `sep38/testdata/golden/` pin the expected output on disk
-rather than in an assertion that somebody could adjust while "fixing" a
-failing test.
+The golden files in `sep38/testdata/golden/` store expected output in
+version-controlled files. The `-update` flag can rewrite them, so review
+the resulting diff against the specification before accepting new values.As per path instructions, 📝 Committable suggestion
Suggested change
🤖 Prompt for AI AgentsSource: Path instructions |
||||||||||||||||
|
|
||||||||||||||||
| | Case | Fee denomination | Why it matters | | ||||||||||||||||
| |:-----|:----------------|:---------------| | ||||||||||||||||
| | Spec worked example (542 BRL → 100 USDC, fee 42 BRL) | sell asset | The spec's own example | | ||||||||||||||||
| | Assets reversed (100 USDC → 500 BRL, fee 10 USDC) | sell asset | The package doc's example | | ||||||||||||||||
| | Buy-asset fee (100 USDC → 500 BRL, fee 10 BRL) | buy asset | The only case where `FeeInBuyAsset` equals `fee.total` exactly | | ||||||||||||||||
|
|
||||||||||||||||
| The first two both denominate in the sell asset — pinning only those two | ||||||||||||||||
| would exercise one branch twice and leave the buy-asset branch unpinned. | ||||||||||||||||
| `TestGoldenCoversBothDenominations` guards this coverage claim. | ||||||||||||||||
|
Comment on lines
+102
to
+104
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Replace “branch” with “denomination case.” The implementation has no Proposed wording-would exercise one branch twice and leave the buy-asset branch unpinned.
+would exercise the sell-asset denomination twice and leave the buy-asset
+denomination unpinned.As per path instructions, 📝 Committable suggestion
Suggested change
🤖 Prompt for AI AgentsSource: Path instructions |
||||||||||||||||
|
|
||||||||||||||||
| --- | ||||||||||||||||
|
|
||||||||||||||||
| ## Related | ||||||||||||||||
|
|
||||||||||||||||
| - `sep38/sep38.go` — the implementation | ||||||||||||||||
| - `sep38/golden_test.go` — the golden-file tests | ||||||||||||||||
| - `sep38/testdata/golden/` — the pinned expected outputs | ||||||||||||||||
| - [SEP-38 spec](https://stellar.org/protocol/sep-0038) — the standard this identity comes from | ||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win ✅ Runtime observed🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- repository conventions ---'
find /tmp/coderabbit-repo-knowledge/wayfare-labs-wayfare-af40d7df -type f -path '*/\*.md' -maxdepth 3 -print 2>/dev/null | sort
printf '%s\n' '--- document context ---'
sed -n '100,118p' docs/sep38-fee-identity.md
printf '%s\n' '--- link status ---'
for url in \
'https://stellar.org/protocol/sep-0038' \
'https://github.com/stellar/stellar-protocol/blob/master/ecosystem/sep-0038.md'; do
printf '%s\n' "$url"
curl -L -sS -o /dev/null -w 'HTTP %{http_code} final=%{url_effective}\n' "$url"
doneRepository: Wayfare-labs/wayfare Length of output: 1136 Repair the SEP-38 specification link. The current URL returns HTTP 404. Replace it with 🤖 Prompt for AI AgentsSources: Path instructions, MCP tools |
||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not compare amounts in different assets as a multiplier.
55.56 BRLis the converted equivalent of10 USDCat the quoted price. The phrase “five times” is misleading because the values have different denominations.Proposed wording
As per path instructions,
docs/**files are contracts, so quoted figures must be technically accurate.📝 Committable suggestion
🤖 Prompt for AI Agents
Source: Path instructions