Skip to content
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

Feature/dutch auction cb bid #1027

Merged
merged 15 commits into from
Jan 15, 2024
Merged

Feature/dutch auction cb bid #1027

merged 15 commits into from
Jan 15, 2024

Conversation

jankjr
Copy link
Contributor

@jankjr jankjr commented Dec 14, 2023

No description provided.

test/utils/bidOnTrade.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@tbrent tbrent left a comment

Choose a reason for hiding this comment

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

Everywhere in the DutchTradeRouter let's use our unit notation to document the type of each variable: https://github.com/reserve-protocol/protocol/blob/master/docs/solidity-style.md#units-in-comments

Copy link
Member

@akshatmittal akshatmittal left a comment

Choose a reason for hiding this comment

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

Looks good to me! Recommended some small changes below, logic looks sound!

contracts/plugins/trading/DutchTradeRouter.sol Outdated Show resolved Hide resolved
contracts/plugins/trading/DutchTradeRouter.sol Outdated Show resolved Hide resolved
contracts/plugins/trading/DutchTradeRouter.sol Outdated Show resolved Hide resolved
contracts/plugins/trading/DutchTradeRouter.sol Outdated Show resolved Hide resolved
contracts/plugins/trading/DutchTrade.sol Outdated Show resolved Hide resolved
contracts/plugins/trading/DutchTrade.sol Outdated Show resolved Hide resolved
contracts/plugins/trading/DutchTradeRouter.sol Outdated Show resolved Hide resolved
contracts/plugins/trading/DutchTradeRouter.sol Outdated Show resolved Hide resolved
contracts/plugins/trading/DutchTradeRouter.sol Outdated Show resolved Hide resolved
contracts/plugins/trading/DutchTradeRouter.sol Outdated Show resolved Hide resolved
contracts/plugins/trading/DutchTradeRouter.sol Outdated Show resolved Hide resolved
Copy link
Collaborator

@tbrent tbrent left a comment

Choose a reason for hiding this comment

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

Nice tests! Unit notation looks good.

Left a comment about a variable I'm not sure is needed in the callback function.

Let's also touch docs/mev.md and at least point them towards the existence of this router, though I think that document should remain describing how to bid against a DutchTrade directly.

contracts/plugins/trading/DutchTrade.sol Outdated Show resolved Hide resolved
contracts/plugins/trading/DutchTradeRouter.sol Outdated Show resolved Hide resolved
@jankjr jankjr force-pushed the feature/dutch-auction-cb-bid branch from 927a7ee to 5cf82e1 Compare January 9, 2024 11:10
@jankjr jankjr force-pushed the feature/dutch-auction-cb-bid branch from 5cf82e1 to 782658d Compare January 9, 2024 11:29
@pmckelvy1
Copy link
Collaborator

1/9/24:

  • add 2nd codepath instead of change bid() interface
  • bid() should be the 3.1.0 version
  • bidWithCallback() is the new function added in this pr

Copy link
Collaborator

@tbrent tbrent left a comment

Choose a reason for hiding this comment

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

The bidType state tracking is uglier than I'd like it to be, but I don't see a good alternative. I tried a few things and none of them are better.

I did leave some fussy comments, though. I think it's important for this contract to be as clear as possible, and there were a few things around the edges I thought detracted from that.

Also, I think we should swap the implementation order of bid() and bidWithCallback(), I think it makes more sense for bid() to come first.

contracts/plugins/trading/DutchTrade.sol Outdated Show resolved Hide resolved
contracts/plugins/trading/DutchTrade.sol Outdated Show resolved Hide resolved
contracts/plugins/trading/DutchTrade.sol Outdated Show resolved Hide resolved
contracts/plugins/trading/DutchTrade.sol Show resolved Hide resolved
contracts/plugins/trading/DutchTrade.sol Outdated Show resolved Hide resolved
contracts/plugins/trading/DutchTrade.sol Outdated Show resolved Hide resolved
Copy link
Collaborator

@pmckelvy1 pmckelvy1 left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Collaborator

@tbrent tbrent left a comment

Choose a reason for hiding this comment

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

LGTM

@pmckelvy1 pmckelvy1 merged commit 5ade2e1 into 3.2.0 Jan 15, 2024
9 checks passed
@pmckelvy1 pmckelvy1 deleted the feature/dutch-auction-cb-bid branch January 15, 2024 20:02
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