[Multi-Chain] Add config file #412
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
since tests need this variable
this should be removed later. we should - add special handling of secrets, or - check this variable at the start of the script
remove token_list again
|
@fhenneke can you rebase once the easy PRs are merged, and in case it is ready for review, let's also remove the "draft" label |
# Conflicts: # tests/unit/test_models.py
|
The things which can already be discussed are:
|
bram-vdberg
reviewed
Nov 6, 2024
this addresses review comments - uses an enum instead of a string for better checking of network values - move initialization of configs into individual classes: use from_network and from_env to initialize classes
mostly by adding comments
fhenneke
added a commit
that referenced
this pull request
Nov 10, 2024
This PR slightly restructures the price fetching for converting token amount for rewards. - Instead of having functions for converting to and from ETH (`eth_in_token`, `token_in_eth`) we just provide an exchange rate for conversion between two tokens (`exchange_rate_atoms`). This will make it easier to add conversion between COW and the native token of the chain, as well as between XDAI and ETH. - Instead of using token ids as argument, the main function accepts addresses. This is mostly to avoid leaking the implementation of the price fetching (token id on coin paprika) into the rest of the code. It also avoids a circular dependency in making the reward token part of the configuration in #412. The `TokenId` abstraction still feels a bit stange as there is some overlap with `Token` due to decimals. Supporting USDC seems a bit artificial since that is only used for testing and checking that decimals are correctly accounted for. Tests for price fetching were adapted accordingly. Transfers for the accounting week starting on 2024-10-29 only differ on the level of floating point precision. --------- Co-authored-by: Haris Angelidakis <[email protected]>
# Conflicts: # src/fetch/payouts.py
# Conflicts: # src/fetch/payouts.py # tests/unit/test_payouts.py
harisang
reviewed
Nov 13, 2024
harisang
reviewed
Nov 13, 2024
harisang
reviewed
Nov 13, 2024
src/config.py
Outdated
| """Initialize node config for a given network.""" | ||
| # Found this exposed infura key on https://rpc.info/ | ||
| infura_key = os.environ.get("INFURA_KEY", "9aa3d95b3bc440fa88ea12eaa4456161") | ||
| node_url = f"https://{network}.infura.io/v3/{infura_key}" |
There was a problem hiding this comment.
PR #430 simplifies a bit this part
There was a problem hiding this comment.
The new environment variable is used now.
harisang
reviewed
Nov 13, 2024
src/config.py
Outdated
| safe_url = ( | ||
| f"https://app.safe.global/{short_name}:{payment_safe_address}" | ||
| ) | ||
| airdrop_url = ( |
There was a problem hiding this comment.
Should we remove this? Seems like an overkill
There was a problem hiding this comment.
I can remove it, as it also did not work for me when I tried it some time ago.
Generally, I think we should provide instructions as part of error messages and logging as much as possible.
# Conflicts: # src/constants.py # src/fetch/transfer_file.py
harisang
reviewed
Nov 15, 2024
# Conflicts: # src/config.py
harisang
approved these changes
Nov 15, 2024
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds a config file. This configuration is supposed to be extended to other chains in future PRs.
This PR adds a config
AccountingConfigwhich consists of multple configurations for different aspects of the accounting.The config is used as a global config. At the moment it is not explicitly passed to any function.
Next steps:
There are still hardcoded constants somewhere in the code. One place is when fetching prices in converting token values, via an ID. The id cannot be part of the config due to circular imports. Instead, an address or id string should be passed and the pricing code should turn it into an appropriate type.
How to review
The first two commits (the names are wrong, I might force push correct ones) add a config and use the config throughout the code. The third commit just removes the old constants file.
One discussion should mostly be about the first commit, i.e. the one adding the configuration classes in
config.py.Another discussion would be around how to move forward from here.