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

Partial restructuring of repo #80

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

Partial restructuring of repo #80

wants to merge 15 commits into from

Conversation

harisang
Copy link
Contributor

This PR attempts to push some functionality out of the TransactionProcessor object and inside the helper classes that are built around it, and specifically the compute_token_imbalances function is now pushed to the RawTokenImbalances class.

This is still in draft mode as more changes might be pushed.


assert all(h == tx_hash for h, _ in transaction_tokens)
assert set(token_address for _, token_address in transaction_tokens) == {
"0x7Fc66500c84A76Ad7e9c93437bFc5Ac33E2DDaE9",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that USDT is removed from the list (it used to be there), as it never touches the settlement contract, so there is definitely no need to include it here.

Maybe we can rename the function itself if you think it is now misleading, but basically it now computes the tokens that are transferred in or out of the settlement contract

Copy link
Contributor

Choose a reason for hiding this comment

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

The new behavior should be fine.

@harisang
Copy link
Contributor Author

@fhenneke Do you think issue #32 needs to be addressed? If yes, i will do it in this PR

@harisang harisang marked this pull request as ready for review October 16, 2024 23:31
Copy link
Contributor

@fhenneke fhenneke left a comment

Choose a reason for hiding this comment

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

Generally looks good. But there are too many changes to check this in detail.

It is a good idea, though, to separate the structural changes from behavioral changes. (Still a bit mixed here, but not much.)

@@ -121,21 +121,28 @@ def extract_actions(self, traces: list[AttributeDict], address: str) -> list[dic
actions.append(dict(action))
return actions

def calculate_native_eth_imbalance(self, actions: list[dict], address: str) -> int:
Copy link
Contributor

Choose a reason for hiding this comment

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

This change looks like a regression in terms of complexity and code style.

What was the rational?

If the change of return type is required, make only that change here, e.g. via if in_flow == 0 and out_flow == 0: return None.

The docstring should mention what it means if None is returned.


assert all(h == tx_hash for h, _ in transaction_tokens)
assert set(token_address for _, token_address in transaction_tokens) == {
"0x7Fc66500c84A76Ad7e9c93437bFc5Ac33E2DDaE9",
Copy link
Contributor

Choose a reason for hiding this comment

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

The new behavior should be fine.

Comment on lines +11 to +14
process_imbalances = os.getenv("PROCESS_IMBALANCES", "True") == "True"
process_prices = os.getenv("PROCESS_PRICES", "False") == "True"
process_fees = os.getenv("PROCESS_FEES", "False") == "True"

Copy link
Contributor

Choose a reason for hiding this comment

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

Using config variables here sounds good.

@fhenneke
Copy link
Contributor

Do you think issue #32 needs to be addressed? If yes, i will do it in this PR

It should be done at some point. If it is easy now, include it. If it is not easy, we can do it separately.

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