Skip to content

trading fee#106

Merged
MathisGD merged 22 commits into
mainfrom
feat/trading-fee-2
Oct 8, 2025
Merged

trading fee#106
MathisGD merged 22 commits into
mainfrom
feat/trading-fee-2

Conversation

@MathisGD

@MathisGD MathisGD commented Sep 25, 2025

Copy link
Copy Markdown
Collaborator

"primitive":

  • trading fee = (bonds-buyerAssets) * fee
  • makerAssets = bonds * price

the computations should be such that

  • feePct = 0 => fee = 0
  • buyerAssets >= sellerAssets

@MathisGD MathisGD self-assigned this Sep 25, 2025
Base automatically changed from feat/linear-price to main September 26, 2025 09:04
@MathisGD MathisGD mentioned this pull request Sep 26, 2025
Comment thread src/MorphoV2.sol Outdated
Comment thread src/Terms.sol Outdated
Comment thread src/Terms.sol Outdated
@MathisGD MathisGD mentioned this pull request Sep 29, 2025
@MathisGD MathisGD requested a review from peyha September 29, 2025 13:07
Comment thread src/MorphoV2.sol
Comment thread src/Terms.sol Outdated
Comment thread src/Terms.sol Outdated
Comment thread src/MorphoV2.sol
@peyha

peyha commented Oct 2, 2025

Copy link
Copy Markdown
Contributor

If we end up using two arguments buyerAssets and sellerAssets, it's probably worth returning both at the end of take, it could simplify integration

Comment thread src/MorphoV2.sol
Comment thread src/MorphoV2.sol
Comment thread src/MorphoV2.sol Outdated
Comment thread src/MorphoV2.sol
This was referenced Oct 2, 2025

@peyha peyha left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

confirming that I have the same formula for sellerAssets,buyerAssets and obligationUnits

@peyha peyha left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The PR is correct for me but I prefer #137's shorter formula

@MathisGD MathisGD mentioned this pull request Oct 7, 2025
@MathisGD MathisGD marked this pull request as draft October 7, 2025 13:31
@MathisGD MathisGD marked this pull request as ready for review October 8, 2025 15:19
@MathisGD MathisGD requested a review from peyha October 8, 2025 15:26
@peyha

peyha commented Oct 8, 2025

Copy link
Copy Markdown
Contributor

The "primitive" in the PR message should be

  • trading fee = (bonds - sellerAssets) * fee
  • makerAssets = bonds * price
    settled during this discussion

@peyha

peyha commented Oct 8, 2025

Copy link
Copy Markdown
Contributor

The justification of the current formula is that we have

  • units * buyerPrice = buyerAssets
  • units * sellerPrice = sellerAssets
  • buyerPrice = sellerPrice (1 - fee) + fee
  • sellerPrice = (buyerPrice - fee) / (1 - fee)

cf this

and that we use buyerAssets = sellerAssets * buyerPrice / sellerPrice and sellerAssets = buyerAssets * sellerPrice / buyerPrice to mitigate rounding issues (all multiplications rounded down)

Comment thread src/MorphoV2.sol
}

require(_isHealthy(offer.obligation, seller), "Seller is unhealthy");
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we could

Suggested change
return (buyerAssets, sellerAssets, obligationUnits)
}

(but we can also raise an issue and do it later)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

yes, will open an other PR!

@QGarchery QGarchery mentioned this pull request Oct 8, 2025
73 tasks

@QGarchery QGarchery left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM. Verified informally the properties in the description of this PR, and also buyerPrice >= sellerPrice (added in the verif wish list)

@MathisGD MathisGD merged commit c8ddd10 into main Oct 8, 2025
3 checks passed
@MathisGD MathisGD deleted the feat/trading-fee-2 branch October 8, 2025 17:14
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.

4 participants