Skip to content

Automation Ethereum #7

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

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Automation Ethereum #7

wants to merge 20 commits into from

Conversation

LauraAntunes1
Copy link
Contributor

Ethereum tool update and automation.

collect_geodata() now depends on the layer(s) specified in config.yaml
Now depends on the layer(s) specified in config.yaml.
Adapted for Ethereum
Removes parts of the code that were used for Bitcoin and are not useful for Ethereum, and adds a get_layers and a get_mode functions.
Updates get_geodata and geography functions to have the same version as for Bitcoin and adds the cluster_organizations function.
Now depends on the layer(s) and mode(s) specified in config.yaml.
New README file.
Gives credit to the Bitnodes project
Python 3.9 or higher is required.
Install dependencies with:

```bash
Copy link
Member

Choose a reason for hiding this comment

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

Is bash needed here? It actually didn't work for me when I tried to run it like this (but the python command on its own works)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

'bash' is just a Markdown command, so the Python command is displayed like it would be in a terminal. When you look at the 'Preview' of the README, it doesn't appear.

```bash
./automation.sh
```
`automation.sh` uses a Python virtual environment. To run it, it is therefore recommended to create one to install all dependencies. The name of this environment is, by default, 'newvenv', but it can be modified in `automation.sh`. Other parameters can be modified in `config.yaml`.
Copy link
Member

Choose a reason for hiding this comment

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

For consistency purposes I think it would make sense to have the same default name for the virtual environment in both cases (while now it's venv for bitcoin and newenv for ethereum). Alternatively, it could be sth explicit, like bitvenv and ethvenv


This component of the project analyses the decentralisation of the Ethereum network by exploring it, collecting information about participating nodes and visualising this information through different graphs. To run it, run:
```bash
./automation.sh
Copy link
Member

Choose a reason for hiding this comment

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

Is this split into two lines on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is the same Markdown command as above. (Explanation: https://thecodebuzz.com/highlight-bash-shell-code-in-markdown-readme-md-wiki-files/)

@@ -1,5 +1,3 @@
#import network_decentralization.protocol as network_proto
#from network_decentralization.constants import MAGIC_NUMBERS, PROTOCOL_VERSIONS
import helper as hlp
import socket
Copy link
Member

Choose a reason for hiding this comment

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

It seems that socket, itertools.repeat and multprocessing are imported but unused

@@ -11,7 +9,7 @@
logging.basicConfig(format='[%(asctime)s] %(message)s', datefmt='%Y/%m/%d %I:%M:%S %p', level=logging.INFO)


def collect_geodata():
def collect_geodata(layers):
logging.info(f'Collecting geodata')
Copy link
Member

Choose a reason for hiding this comment

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

No need for f-string if there are no variables (same below)


mode:
- Countries
- Organizations

execution_parameters:
concurrency: 100

# Paths to directories of snapshot db files; either absolute or relative from run.py.
Copy link
Member

Choose a reason for hiding this comment

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

I think this comment is from an old file, as it doesn't seem relevant here.

@@ -1,19 +1,29 @@
import json
import re
Copy link
Member

Choose a reason for hiding this comment

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

Unused import

@@ -10,10 +10,11 @@
logging.basicConfig(format='[%(asctime)s] %(message)s', datefmt='%Y/%m/%d %I:%M:%S %p', level=logging.INFO)
Copy link
Member

Choose a reason for hiding this comment

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

There seem to be some unused imports (numpy, json, networkx, time)

@@ -1,13 +1,10 @@
#from network_decentralization.constants import DEFAULT_PORTS
import shutil
Copy link
Member

Choose a reason for hiding this comment

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

I think this is unused, and also json and pandas

@@ -66,34 +72,25 @@ def get_layer(line):
return 'Consensus'
Copy link
Member

Choose a reason for hiding this comment

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

Is the space needed here? Is there any chance that this ignores occurences that have the "eth" part but without the space?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so

Adds docstrings and deletes useless imports and function
Adds docstrings and deletes useless imports
Adds docstrings and removes useless imports
Creates output directory if it doesn't exist
Changes virtual environment's name
Changes virtual environment's name
Removes outdated comment
Removes spaces
Removes f-strings
Removes unused import
Removes unused import
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