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

cli: refactor sanitizeMinBid #641

Merged
merged 5 commits into from
Apr 4, 2024

Conversation

MariusVanDerWijden
Copy link
Collaborator

Refactor sanitizeMinBid

cli/main.go Outdated Show resolved Hide resolved
cli/main.go Outdated
return nil, errors.New("minimum bid is too large, please ensure min-bid is denominated in Ethers")
}
if minBid > 0.0 {
log.Infof("Minimum bid: %v eth", minBid)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: should log after error checking and should not be in sanitizeMinBid

avalonche
avalonche previously approved these changes Apr 2, 2024
@MariusVanDerWijden
Copy link
Collaborator Author

The linter fails, because it complains about dynamic errors and enforces static errors. I kinda disagree with the linter. Declaring static errors makes everything more spaghetti and this method is not used from a context where the type checking you get with static errors is useful. I am going to declare them as static anyway, to make the linter happy, but I think it makes the code worse and you should consider disabling the err113 check

jtraglia
jtraglia previously approved these changes Apr 3, 2024
Copy link
Collaborator

@jtraglia jtraglia left a comment

Choose a reason for hiding this comment

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

LGTM, just one suggestion around min bid log.

cli/main.go Outdated
Comment on lines 186 to 183
log.Infof("minimum bid: %v eth", *relayMinBidEth)
}

relayMinBidWei, err := common.FloatEthTo256Wei(*relayMinBidEth)
relayMinBidWei, err := sanitizeMinBid(*relayMinBidEth)
if err != nil {
log.WithError(err).Fatal("failed converting min bid")
log.WithError(err).Fatal("Failed sanitizing min bid")
}
if relayMinBidWei.BigInt().Sign() > 0 {
log.Infof("Min bid set to %v wei", relayMinBidWei)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This changes the log from Eth to Wei. Maybe we should log both.

log.Infof("Min bid set to %v eth (%v wei)", relayMinBidEth, relayMinBidWei)

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 52.30%. Comparing base (8b662e0) to head (8099445).

Files Patch % Lines
cli/main.go 0.00% 13 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #641      +/-   ##
===========================================
- Coverage    52.37%   52.30%   -0.07%     
===========================================
  Files           14       14              
  Lines         1600     1602       +2     
===========================================
  Hits           838      838              
- Misses         703      706       +3     
+ Partials        59       58       -1     
Flag Coverage Δ
unittests 52.30% <0.00%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@metachris metachris left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

About the error linter -- you making a strong case definitely means a lot to me and I'd like to consider it. Do you feel strongly enough to create an issue on this repo, to continue the conversation there?

@metachris metachris merged commit 5994642 into flashbots:develop Apr 4, 2024
2 checks passed
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.

5 participants