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

Audit branch #2

Merged
merged 48 commits into from
Nov 18, 2024
Merged

Audit branch #2

merged 48 commits into from
Nov 18, 2024

Conversation

Evan-Kim2028
Copy link
Collaborator

  • Refactored the env variables and flags to use urfav cli v2 library.
  • Refactored repository to reorganize around main.go and everythig else in "internal" package
  • removed self creation of hTTP client
  • check return body contents in sendBundle function
  • replaced context.Background() with context.WithTimeout
  • added contract addresses as env variables

@Evan-Kim2028 Evan-Kim2028 self-assigned this Oct 31, 2024
@Evan-Kim2028
Copy link
Collaborator Author

  • Added unit tests coverage for bidderapi.go functions sendPreconfBid and sendBid. These are the most important functions and will likely change the most over time in future releases
  • switched geth logging for zerolog logging
  • refactored environment variables for more simplicity, simplifed main.go logic
  • refactored sendBid function to be more modular

Dockerfile Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
main.go Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
@Evan-Kim2028
Copy link
Collaborator Author

  • used more context timeout modifications
  • changed logging from zerolog to standard library logging
  • refactor environment variable handling
  • updated go mod and version
  • refactor minor formatting and reduce logic complexity

Dockerfile Outdated Show resolved Hide resolved
internal/eth/bundle.go Outdated Show resolved Hide resolved
internal/eth/bundle.go Outdated Show resolved Hide resolved

// init initializes the defaultTimeout variable by reading the DEFAULT_TIMEOUT
// environment variable. If not set or invalid, it defaults to 15 seconds.
func init() {
Copy link

Choose a reason for hiding this comment

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

I would suggest creating a service struct in which you set all the necessary options that are known at the time of creation and will not change (e.g. default timeout, rpcurl, etc.) and fields that must maintain some state (e.g. logger, ethclient, httpclient, etc.). Then attach existing functions to this structure. This will result in fewer parameters in these methods and avoid global state. Your code will also be easier to mock and test.

)

// Define flag names as constants
const (
Copy link

Choose a reason for hiding this comment

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

Instead of this, I'd suggest doing the following:

var (
    bidderAddressFlag := &cli.StringFlag{
        Name:    "bidder-address",
        Usage:   "Address of the bidder",
        EnvVars: []string{"BIDDER_ADDRESS"},
        Value:   "mev-commit-bidder:13524",
     },
     // other flags...
)

// Now below you can do:
c.String(bidderAddressFlag.Name)

main.go Outdated
&cli.StringFlag{
Name: FlagEnv,
Usage: "Path to .env file",
EnvVars: []string{"ENV_FILE"},
Copy link

Choose a reason for hiding this comment

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

Consider using some sort of prefix (maybe PRECONF_BOT) for all env variables to avoid collisions or so that they already exist.

Value: 15, // Default to 15 seconds
},
},
Action: func(c *cli.Context) error {
Copy link

Choose a reason for hiding this comment

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

The c.Context should be used as parent context in all places where you create a new context. This will help you shutdown the app gracefully.

internal/mevcommit/client.go Outdated Show resolved Hide resolved
internal/mevcommit/client.go Outdated Show resolved Hide resolved
@Evan-Kim2028 Evan-Kim2028 merged commit 3cdc40f into main Nov 18, 2024
1 check passed
@Evan-Kim2028 Evan-Kim2028 deleted the audit-branch branch November 18, 2024 06:55
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.

2 participants