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

Redstone OEV dAppControl update #435

Merged
merged 7 commits into from
Sep 24, 2024

Conversation

jj1980a
Copy link
Contributor

@jj1980a jj1980a commented Sep 23, 2024

No description provided.

Comment on lines 202 to 205
SafeTransferLib.safeTransferETH(
RedstoneOevDappControl(_control()).oevAllocationDestination(),
bidAmount - _oevShareBundler - _oevShareFastlane
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should calculate the result of bidAmount - _oevShareBundler - _oevShareFastlane and only if not zero, do the transfer call. Looks like it could be zero from the restrictions on the setter functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in c47999f.

uint256 public oevShareBundler;
uint256 public oevShareFastlane;

address public oevAllocationDestination;

mapping(address bundler => bool isWhitelisted) public bundlerWhitelist;
uint32 public NUM_WHITELISTED_BUNDLERS = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could name using camel case as all caps kinda implies its a constant, but it is not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in c8f98c9.


mapping(address bundler => bool isWhitelisted) public bundlerWhitelist;
uint32 public NUM_WHITELISTED_BUNDLERS = 0;

mapping(address oracle => bool isWhitelisted) public oracleWhitelist;
uint32 public NUM_WHITELISTED_ORACLES = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could name using camel case as all caps kinda implies its a constant, but it is not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in c8f98c9.

Copy link
Contributor

@BenSparksCode BenSparksCode left a comment

Choose a reason for hiding this comment

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

LGTM to merge into redstone_oev_dapp_control. Will need to sync that branch with main after merging as it looks a bit out of date. That will probably resolve the CI issues.

Also should remove openzeppelin-contracts-upgradeable lib unless we really need it.

@@ -52,31 +65,49 @@ contract RedstoneOevDappControl is DAppControl {
trustedOpHash: false,
invertBidValue: false,
exPostBids: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make this ex-post-bids? That way it is easier for the solver

@jj1980a jj1980a merged commit 782489e into redstone_oev_dapp_control Sep 24, 2024
1 of 3 checks passed
@jj1980a jj1980a deleted the redstone_oev_dapp_control_2 branch September 24, 2024 05:52
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.

3 participants