initial refactor #1

jalbrekt85 · 2024-06-13T11:19:33Z

there are 3 main files to review, rest are peripheral:

overview

`core_pools.py`: holds the `CorePoolData` and `CorePool` class

CorePoolData: organizes and holds data fetched from subgraph. the defined fields represent the parts of the core pool data that are immutable (unlike CorePool). this is also the data structure that gets cached
CorePool: initialized with its respective CorePoolData. defines initial incentive split to various sources (to dao, to aura, to bal, etc). these attributes are subject to change later in redistribution logic in FeeAllocator. CorePool also has access to its respective Chain class, so it can access chain wide data, like aggregate core pool fees for a chain

`chains.py`: holds the `Chain` and `Chains` class:

Chains: initialized with InputFees (ex.) and a date range over which to allocate fees. initializes a list of Chains based on the chains/fees defined in InputFees and contains chain agnostic data like the fee config and aura vebal share
Chain: this class contains the bulk of the core pool data collection process. uses the subgraph to create a list of CorePoolData and handles caching (based on chain/timestamp). also contains chain wide properties like total fees earned and can reference the Chains class for ecosystem wide data

`fee_allocator.py`: holds the `FeeAllocator` class

FeeAllocator: initializes the Chains class and as a result, holds the CorePools for every chain. the redistribute_fees method will contains the all of the redistribution logic which modifies these CorePools based on the initial split. the remaining methods are for generating csvs/payloads based on the final CorePool data

important parts to review

Chain helper methods that facilitate the core pool data collection process like _fetch_and_process_core_pool_data and _should_add_core_pool. review that pools are being filtered correctly and fetched data is being appropriately attributed to each pool. all of the models for the data fetched from the subgraph is defined in bal_tool's models.py which can be helpful for reviewing this part
CorePoolsetter methods and math. make sure the initial allocations amounts to each source are correct
FeeAllocator's redistribute_fees and _handle_aura_min methods. lots of bulky logic that modifies core pool attributes. if the allocation numbers are ever off, its probably from these methods.
general math and rounding
follows https://forum.balancer.fi/t/bip-457-core-pool-incentive-program-automation/5254

setup, run and test instructions are in readme

fee_allocator/accounting/core_pools.py

Tritium-VLK · 2024-08-30T17:23:55Z

Can you please provide architecture documentation that explains the flow of the code.

Where/what is the the pipeline/stages. What are each of the steps, where are they handled, why?
Why does chains.py kind of do half the processing on init, and then there are other functions for redistro.
How can I easily understand the flow?
How do these objects/classes pertain to other things in Balancer and how can they be reused? Are things grouped in a way that makes them reusable, or is there just a bunch of extra structure here for the sake of it?

Tritium-VLK · 2024-09-03T18:06:04Z

fee_allocator/accounting/chains.py

+
+
+class CorePoolChain(AbstractCorePoolChain):
+    """


For the major classes, can we also document each of the inputs for init, and also maybe the ways that it is intended to be used.

This class only has 2 non-internal functions. I imagine that the intention is that it is accessed through some of the variables defined on init. These variable that have some kind of derived data are also in a sense external facing functions that should be described.

fee_allocator/accounting/core_pools.py

Tritium-VLK · 2024-09-03T18:59:37Z

fee_allocator/accounting/core_pools.py

+    total_earned_fees_usd: Decimal = field(init=False)
+
+    def __post_init__(self):
+        self.address = self._set_address()


Same here. As each of these vars are a result of running some logic/processing, would be good to document them and make those docs as easy to find as possible working with this object.

Tritium-VLK · 2024-09-04T00:31:03Z

main.py

+        input_fees = json.load(f)
+
+    date_range = (ts_in_the_past, ts_now)
+


It would be nice to take a reader step by step through the process of ingesting the data, allocating collected fees and then redisitributed based on various logic here in the main loop. I still think it could help to move away from nesting classes so deep and be a little more verbose here in main.

Tritium-VLK

Ok. So I think I got through most of the underlying stuff with some comments. Let's get this all documented/ well understood/refactored and then it should be easier to understand the logic/really debug the code.

Tritium-VLK · 2024-09-04T10:41:26Z

fee_allocator/accounting/models.py

+    vote_incentive_pct: Decimal
+
+
+class RawPoolFeeData(BaseModel):


Classes like this that contain other classes/complex objects need documentation that explain their purpose and structure.

fee_allocator/accounting/models.py

Tritium-VLK · 2024-09-04T10:44:23Z

fee_allocator/accounting/overrides.py

+from fee_allocator.constants import POOL_OVERRIDES_URL
+import requests
+
+if TYPE_CHECKING:


What does type checking have to do with importing pool fees? Maybe a comment about what these lines are doing?

TYPE_CHECKING does static type checking, please read docs first: https://docs.python.org/3/library/typing.html#typing.TYPE_CHECKING

the reason we do this instead of importing normally is because it will cause a circular import error, i guess that can be reflected as comment there

Tritium-VLK · 2024-09-04T10:48:20Z

fee_allocator/accounting/overrides.py

+overrides_data = requests.get(POOL_OVERRIDES_URL).json()
+
+
+class PoolFeeOverrideMeta(ABCMeta):


All these overrides are quite complex. I think it'd be good to spend some time foucsing on docuemntation here. What do all these classes/functions do? Why are they needed/how do they relate to other things? Are they setting defaults, what are the defaults/where can they be found?

this was implemented when it wasnt clear how much fine tuned control we wanted over the override config, but ya agree we can simplify this a lot, can probably just be a dict assuming we dont need finer control of the override allocations than what is needed now

Tritium-VLK · 2024-09-04T10:49:32Z

fee_allocator/accounting/overrides.py

+        pass
+
+
+class RethWethOverride(PoolFeeOverride):


I don't think I like the idea of building individual overrides as classes. It's better to have them grouped by type, and then a single json file where they can all be easily seen in config.

Overrides are config not code, there needs to be well documented code to handle the config, but the overrides themselves should remain in json files, and likely in another repo like multisig-ops with different change control.

yes assuming we just want the overrides to be static, all or nothing overrides like we have now, then this isnt necessary. i kept it in because i thought having the extra control would be nice if needed in the future, but might not be worth if it is making it too complex

fee_allocator/constants.py

Tritium-VLK · 2024-09-04T10:53:41Z

fee_allocator/accounting/decorators.py

+from decimal import Decimal
+
+
+def return_zero_if_dust(threshold=Decimal("1E-20"), any_or_all="any"):


Why do we want to do this? Is this about rounding down, we should be very very considerate of rounding logic, it should always be quite apparent what is happening. I don't think I see this being used anywhere, can it be removed? Would prefer rounding was more explicit where it happened.

originally i decorated most of the PoolFee and CorePoolChain attributes/properties that returned decimals to use this to avoid having a bunch of repetitive code on each method, but ultimately chose to do the rounding at the end in the FeeAllocator. it is not used anywhere atm, but i didnt see any harm in deleting it

Tritium-VLK · 2024-09-04T10:55:42Z

fee_allocator/accounting/decorators.py

+                raise TypeError(
+                    f"`round` can only be used on methods that return `Decimal`, not {type(result)}"
+                )
+            return result.quantize(Decimal(10) ** -decimals)


Would be nice to define a return type here. I'm confused by this function. Does it output a decimal and take a decimal. I'd actually prefer the .quantize() or other rounding operation to very visible and not buried deep in class structure.

Tritium-VLK · 2024-09-04T10:56:24Z

fee_allocator/accounting/decorators.py

@@ -0,0 +1,56 @@
+from functools import wraps


See comments below. I don't think these decorators or this style of handling rouding makes sense.

Tritium-VLK · 2024-09-04T10:58:11Z

fee_allocator/accounting/interfaces.py

+from abc import ABC
+
+"""
+`CorePoolChain` and `PoolFee` reference each other so they need to be abstract to prevent circular imports.


Wonder if there is a way we can do this without circural dependancies and needing to get into abstract classes. I think I understand why this is happening, but it's confusing and hard to think about and leads to more jumping around in the code when trying to debug.

Would be really good to come up with an architecture diagram or something and then think about the structure. Maybe I'llt ry to get a decent UML output.

we do this so that:

core pools are aware of chain data

chains are aware of core pool data

which is needed for doing a lot of the fee split calculations and makes it a lot cleaner when accessing data later in the FeeAllocator methods

gosuto-inzasheru · 2025-01-20T15:19:51Z

@jalbrekt85 can you make sure this can be closed?

jalbrekt85 · 2025-01-22T14:10:24Z

relevant parts of this pr are already included in biweekly-runs branch, good to close

jalbrekt85 requested review from gosuto-inzasheru and Tritium-VLK June 13, 2024 11:38

Tritium-VLK reviewed Aug 21, 2024

View reviewed changes

fee_allocator/accounting/core_pools.py Outdated Show resolved Hide resolved

Tritium-VLK reviewed Aug 21, 2024

View reviewed changes

fee_allocator/accounting/core_pools.py Outdated Show resolved Hide resolved

Tritium-VLK reviewed Aug 21, 2024

View reviewed changes

fee_allocator/accounting/core_pools.py Outdated Show resolved Hide resolved

gosuto-inzasheru assigned jalbrekt85 Aug 26, 2024

Xeonus assigned Tritium-VLK Sep 2, 2024

Tritium-VLK reviewed Sep 4, 2024

View reviewed changes

Tritium-VLK suggested changes Sep 4, 2024

View reviewed changes

jalbrekt85 added 9 commits November 3, 2024 23:15

add collect fees workflow and most recent fee data

07c9704

bump requirements

4b2bf4e

handle wei fees

e49c9f9

adjust doa/vebal share based on original dist

d42ce13

11-07 fees

2adda31

11-21 fees

d139b74

add _filter_dusty_bal_incentives

9e98651

bump bal addresses

f525308

bump reqs

583f1b7

jalbrekt85 changed the base branch from main to biweekly-runs December 5, 2024 07:26

Merge branch 'biweekly-runs' into initial-refactor

4cb577f

jalbrekt85 closed this Jan 22, 2025

jalbrekt85 deleted the initial-refactor branch January 22, 2025 14:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

initial refactor #1

initial refactor #1

jalbrekt85 commented Jun 13, 2024 •

edited

Loading

Tritium-VLK commented Aug 30, 2024

Tritium-VLK Sep 3, 2024

Tritium-VLK Sep 3, 2024

Tritium-VLK Sep 4, 2024

Tritium-VLK left a comment

Tritium-VLK Sep 4, 2024

Tritium-VLK Sep 4, 2024

jalbrekt85 Sep 4, 2024 •

edited

Loading

Tritium-VLK Sep 4, 2024

jalbrekt85 Sep 4, 2024 •

edited

Loading

Tritium-VLK Sep 4, 2024

jalbrekt85 Sep 4, 2024 •

edited

Loading

Tritium-VLK Sep 4, 2024

jalbrekt85 Sep 4, 2024

Tritium-VLK Sep 4, 2024

Tritium-VLK Sep 4, 2024

Tritium-VLK Sep 4, 2024

jalbrekt85 Sep 4, 2024 •

edited

Loading

gosuto-inzasheru commented Jan 20, 2025

jalbrekt85 commented Jan 22, 2025

		input_fees = json.load(f)

		date_range = (ts_in_the_past, ts_now)

		overrides_data = requests.get(POOL_OVERRIDES_URL).json()


		class PoolFeeOverrideMeta(ABCMeta):

		from decimal import Decimal


		def return_zero_if_dust(threshold=Decimal("1E-20"), any_or_all="any"):

initial refactor #1

initial refactor #1

Conversation

jalbrekt85 commented Jun 13, 2024 • edited Loading

there are 3 main files to review, rest are peripheral:

overview

core_pools.py: holds the CorePoolData and CorePool class

chains.py: holds the Chain and Chains class:

fee_allocator.py: holds the FeeAllocator class

important parts to review

Tritium-VLK commented Aug 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tritium-VLK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jalbrekt85 Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jalbrekt85 Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jalbrekt85 Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jalbrekt85 Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

gosuto-inzasheru commented Jan 20, 2025

jalbrekt85 commented Jan 22, 2025

jalbrekt85 commented Jun 13, 2024 •

edited

Loading

`core_pools.py`: holds the `CorePoolData` and `CorePool` class

`chains.py`: holds the `Chain` and `Chains` class:

`fee_allocator.py`: holds the `FeeAllocator` class

jalbrekt85 Sep 4, 2024 •

edited

Loading

jalbrekt85 Sep 4, 2024 •

edited

Loading

jalbrekt85 Sep 4, 2024 •

edited

Loading

jalbrekt85 Sep 4, 2024 •

edited

Loading