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

fix fre/dispenser monitor #919

Merged
merged 7 commits into from
Oct 13, 2022
Merged

fix fre/dispenser monitor #919

merged 7 commits into from
Oct 13, 2022

Conversation

alexcos20
Copy link
Member

@alexcos20 alexcos20 commented Oct 13, 2022

Critical issues resolved:

  • use fre/dispenser address that generated the event (and not something that we have in our addresses.json). What will happen when we will have multiple fre/dispenser templates? -> Aquarius down
  • check if that fre/dispenser is approved by our Factory
  • when fetching fre info/dispenser info, use try / catch. Any web3 call may fail -> Aquarius down

Major changes were done in handle_price_change , in events_monitor.py

Also, improved logging a little bit

@alexcos20 alexcos20 self-assigned this Oct 13, 2022
@alexcos20 alexcos20 marked this pull request as ready for review October 13, 2022 13:18
@@ -105,7 +105,7 @@ def read(self, resource_id):
:param resource_id: id of the object to be read.
:return: object value from elasticsearch.
"""
logger.debug("elasticsearch::read::{}".format(resource_id))
# logger.debug("elasticsearch::read::{}".format(resource_id))
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a log here as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, this will flood the console with reads for non-existing assets because of non-ocean NFT transfers

@alexcos20 alexcos20 changed the title fixes fixe fre/dispenser monitor Oct 13, 2022
@alexcos20 alexcos20 changed the title fixe fre/dispenser monitor fix fre/dispenser monitor Oct 13, 2022
@codeclimate
Copy link

codeclimate bot commented Oct 13, 2022

Code Climate has analyzed commit b00b138 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 73.6% (50% is the threshold).

This pull request will bring the total coverage in the repository to 89.7% (-0.8% change).

View more on Code Climate.

@alexcos20 alexcos20 merged commit 3c763b9 into main Oct 13, 2022
@alexcos20 alexcos20 deleted the bug/fix_fre_and_dispensers branch October 13, 2022 13:33
@@ -172,7 +174,12 @@ def process_current_blocks(self):
self.retry_mechanism.process_queue()

last_block = self.get_last_processed_block()

Choose a reason for hiding this comment

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

instead of the while True loop couldn't we do a isinstance(last_processed_block, int)? a while True nested in 2 try / except can have unexpected behaviors

current_block = self._web3.eth.block_number
current_block = None
try:
current_block = self._web3.eth.block_number
Copy link

@juanpablovs juanpablovs Oct 13, 2022

Choose a reason for hiding this comment

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

depending on the congestion of the network this can consistenly fail, as block_number usually returns a promise that ultimately resolves to a string. Coroutines in python could solve this. What do you think?

Copy link

@juanpablovs juanpablovs left a comment

Choose a reason for hiding this comment

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

made a couple of comments that perhaps we can discuss when thinking about reviewing (and separating responsibilities) in this

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