grant review feedback

[zeroXbrock]

posting this issue here for reviewers to leave comments

[ZigaMr]

Hello, I just updated the readme with new client initalization options. It uses either the init constructor or the classmethod. They are both desribed in the Usage section.

[ZigaMr]

Also some thoughts:

• The client expects a config.json file with necessary parameters, should I update it to use environment variables?
• For the SSE client I used an existing library https://pypi.org/project/aiohttp-sse-client/ which I intend to credit. Let me know if you would like an alternative.
• Under simulate_bundle, the method waits for tx to land onchain using web3.py wait_for_transaction_receipt() AND get_transaction() which uses an (probably) unnecessary RPC call (2 instead of 1). Since wait_for_transaction_receipt() returns insuficient data to calculate a raw sign tx it is necessary to do it this way. Let me know of any other web3.py functions that might skip this step

[zeroXbrock]

re @ZigaMr

• The client expects a config.json file with necessary parameters, should I update it to use environment variables?

• We should definitely remove config.json as well as MevShareClient.from_config. Relying on a json file holding a private key is risky, and not worth the convenience. Best to let users write their own config abstractions. Also just a general rule of thumb, if your code doesn't absolutely have to read files from disk, don't (basically only file-IO libs and user-facing apps should read files). Rather, require the client user to provide the already-deserialized file contents; outsource the file handling. Differences in the user's execution environment (async runtime, app permissions on the system, etc) can easily break your file-handling logic.
• Environment variables aren't really suited to a client library. It would make sense to use them for a cli application, but a client library should not be reading from the environment (this is generally a security red flag).
• The default args in the constructor do the job well -- let's just limit it to that.

• For the SSE client I used an existing library https://pypi.org/project/aiohttp-sse-client/ which I intend to credit. Let me know if you would like an alternative.

this looks fine, couldn't find a better alternative

• Under simulate_bundle, the method waits for tx to land onchain using web3.py wait_for_transaction_receipt() AND get_transaction() which uses an (probably) unnecessary RPC call (2 instead of 1). Since wait_for_transaction_receipt() returns insuficient data to calculate a raw sign tx it is necessary to do it this way. Let me know of any other web3.py functions that might skip this step

Yeah, you don't need the transaction receipt. A plain transaction from provider.get_transaction is sufficient to reconstruct the tx for simulation, which is all we need.

[zeroXbrock]

@ZigaMr I also had just a couple comments on the readme: one note here, and the others I'll just make a PR for.

The examples all seem to imply that the user would create a new client for every request. Rather, we should try to suggest that the client is instantiated once and passed as an argument to functions that use it.

so instead of

async def build_and_send():
    client = MevShareClient.from_config(network='goerli', config_dir='../config.json')
    # ...
    print(await client.send_bundle(params))

we should have something like

async def build_and_send(client: MevShareClient):
    # ...
    print(await client.send_bundle(params))

so the user can just refer to the example of how to instantiate a MevShareClient, and we don't have to keep rewriting it.

not necessarily a big deal but it does feel cleaner, and suggests the user to implement best practices

[zeroXbrock]

@ZigaMr posted a PR w/ some readme tweaks here: https://github.com/ZigaMr/mev-share-py/pull/2/files

[zeroXbrock]

@ZigaMr on the custom RLP encoder, I think you can use what your eth provider already has. From web3.py:

haven't tested, but looks like this function gives you the format you need to call signed_txn.rawTransaction which will give you the serialized tx you need

[ZigaMr]

Hey @zeroXbrock thanks for the feedback, I'm just going to comment all the updates I made:

• config.json and classmethod are removed, all initialization is done normally through the constructor now

• I updated the Readme with correct examples and left the tx placeholders in the format '<user_tx_hash>' and '<raw_signed_tx>' since I wanted to leave a clear example for the user. The python scripts in the examples folder have been updated though, for instance, simulate_bundle.py now shows how to read from SSE and send/simulate the bundle in the callback function.

• Regarding the RLP encoding, I think my explanation was a bit confusing. So what we are encoding is the tx that we receive from the SSE for which we have no private key, just the v/r/s part of the signature (once it lands onchain). Now while the format_transaction does help, I can't use the sign_transaction part since it expects the private key.

• get_transaction() does return all the necessary data to reconstruct the raw signed tx, but wait_for_tx_receipt() waits for tx to be mined. So just using get_transaction() would return an error while we are still waiting for the tx to be confirmed. So I left both functions alone for now.

[ZigaMr]

Another important change, as per your suggestion, the custom callbacks are now expected to receive pendingTx/pendingBundle parameter as well as MevShareClient object which is just a reference to the already initialized client. You can review it in the latest commit.

[zeroXbrock]

Looking better, @ZigaMr! I took another look and found a few more things:

• end of readme is missing content (faq, acknowledgements, etc)
• missing .gitignore, includes build artifacts
  • I went ahead and fixed this. PR here
• three examples don't compile -- they include placeholders like '<private_key>'
  • these need to run without need for manual changes. use environment variables in the examples to
    replace the placeholders