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

Formating prints according to CERT-7899 #61

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

Conversation

liav-certora
Copy link
Contributor

No description provided.

@liav-certora liav-certora requested a review from a team as a code owner January 1, 2025 16:16
@nivcertora nivcertora self-requested a review January 2, 2025 10:01
Copy link
Collaborator

@nivcertora nivcertora left a comment

Choose a reason for hiding this comment

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

Price_feed Design issue that needs to be decided.

Comment on lines 107 to 130
coingecko_name = CoinGeckoAPI().get_name()
token_validation_res = {r for r in overall_verified_vars if r.found_on == coingecko_name}
price_feed_validation_res = overall_verified_vars - token_validation_res

# Print price feed validation
pp.pprint('Price feed validation', pp.Colors.INFO)
msg = (f'{len(price_feed_validation_res)}/{num_addresses} '
'were identified as price feeds of the configured providers:\n')
for i, var_res in enumerate(price_feed_validation_res, 1):
msg += (f'\t{i}. {var_res.address} found on {var_res.found_on}\n'
f"\t Name: {var_res.price_feed['name']}\n"
f"\t Decimals: {var_res.price_feed['decimals']}\n")
pp.pprint(msg, pp.Colors.SUCCESS)

# Print token validation
pp.pprint('Token validation', pp.Colors.INFO)
msg = (f'{len(token_validation_res)}/{num_addresses} '
'were identified as tokens of the configured providers:\n')
for i, var_res in enumerate(token_validation_res, 1):
msg += (f'\t{i}. {var_res.address} found on {var_res.found_on}\n'
f"\t Name: {var_res.price_feed['name']}\n"
f"\t Symbol: {var_res.price_feed['pair']}\n"
f"\t Decimals: {var_res.price_feed['decimals']}\n")
pp.pprint(msg, pp.Colors.SUCCESS)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we decide to go with that insanity eventually, lets do it properly by split the logic for the entry point to the check in order to achieve a general solution which will support additional providers by not import them and check for their name every time. @yoav-el-certora Feel free to decide whether you want this change as part of this PR or open different ticket which i can take later.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that I commented/asked this in your previous PR when splitting providers to price feed and tokens.
Lets create a subtask for this.
Great point

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 I actually wanted to do this but didn't want to take it too far though in this ticket so it wouldn't get prolonged. I aim to do at as another ticket later @yoav-el-certora.

Copy link
Collaborator

@nivcertora nivcertora left a comment

Choose a reason for hiding this comment

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

Replace dict usage with already given object usage.

Comment on lines 44 to 47
'''
address: str
found_on: str
price_feed: dict
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use the original pydantic class insead of the dict as its more structured approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

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.

3 participants