Repository navigation
fix(facade): report TradeHelperFacet sellAmount in token quanta - #1306
Conversation
getAllOpenTradesForRToken returned DutchTrade.sellAmount(), a {sellTok} D18 value, next to bidAmount in {qBuyTok}. Use lot() so both amounts are in token quanta, matching the amount createTrustedFill uses.
Amp-Thread-ID: https://ampcode.com/threads/T-01a1218b-431d-74f9-94c8-a47fdaadb450
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review. 📝 WalkthroughWalkthrough
ChangesTrade helper updates
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~5 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The facade now reports the sell-token amount used by the in-repository fill paths. No merge-blocking regression is established. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
TradeHelperFacet.getAllOpenTradesForRTokenreturns the two amounts of eachSingleBidin different units:sellAmountcame fromDutchTrade.sellAmount(), which is{sellTok}, an 18-decimal fixed-point value.bidAmountcomes fromDutchTrade.bidAmount(), which is{qBuyTok}, buy-token quanta.For a sell token with fewer than 18 decimals,
sellAmountis too large by10^(18 - decimals). The trusted fillers SDK uses this value as the CoW order sell amount, soCowSwapFillerrejects the order (OrderCheckFailed(9)). See reserve-protocol/reserve-fillers-monorepo#26 for the SDK-side workaround.This change reports
trade.lot(), which is{qSellTok}. It is the same amountDutchTrade.createTrustedFillgives the filler. The struct fields now have unit comments.Compatibility
SingleBid[]and break existing decoders, so this PR changes the value only.lot()returns the same number as before.TradeHelperFacetis deployed and the Facade owner callssave(newFacet, [getAllOpenTradesForRToken.selector])on each chain.Verification
hardhat compilepasses.prettier --checkandsolhintpass on the changed file. No tests were added.Summary by CodeRabbit