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

Remove dead code #413

Merged
merged 5 commits into from
Nov 4, 2024
Merged

Remove dead code #413

merged 5 commits into from
Nov 4, 2024

Conversation

fhenneke
Copy link
Collaborator

@fhenneke fhenneke commented Nov 4, 2024

The PR removes some code which is not used at the moment.

  • token_list.py is not a concept we currently use in our accounting. We could add checks for that back into the accounting, but it looks more like a circuit breaker job than the job of the accounting.
  • test_internal_trades.py was testing internal trades, which is not part of the accounting anymore. It has only been testing properties which are not true anymore. Some of those tests could be refactored into tests for slippage. But for now I think it is fine to remove it. I also removed a reference in test_models.py.
  • dataset.py was not used at all.
  • gap_detector.py looks like interesting functionality. But it will probably be superseded by whatever we build for dune-sync. We have not used the functionality at least in the last year. So it should be fine to remove it.

a list of tokens permitted for slippage was used at some point, but not anymore
the function was not used anymore in the code
tests were removed as well
is has not been used at all in the last year,
it might still work but might also not.
Copy link
Contributor

@harisang harisang left a comment

Choose a reason for hiding this comment

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

Makes sense at a first glance. If you have tested it and nothing breaks, then i am fine with it!

@fhenneke
Copy link
Collaborator Author

fhenneke commented Nov 4, 2024

I am currently running payments for last week. If results coincide with what we got with main, I will merge this.

@fhenneke
Copy link
Collaborator Author

fhenneke commented Nov 4, 2024

I only see differences around the level of machine precision. Not sure where they are coming from, but definitely not something I am worried about.

So I am merging this. We should have another look when the dry run happens on Thursday.

@fhenneke fhenneke merged commit 14608bb into main Nov 4, 2024
5 checks passed
@fhenneke fhenneke deleted the remove_dead_code branch November 4, 2024 17:04
@github-actions github-actions bot locked and limited conversation to collaborators Nov 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants