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.

Sign up for GitHub

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

Jump to bottom

Hotfixes, just fresh from the oven! #2375

Merged
merged 3 commits into from
Oct 16, 2020
Merged

Hotfixes, just fresh from the oven! #2375

merged 3 commits into from
Oct 16, 2020

Conversation

cygnusv
Copy link
Member

@cygnusv cygnusv commented Oct 16, 2020

No description provided.

derekpierre
derekpierre reviewed Oct 16, 2020
View reviewed changes
nucypher/blockchain/eth/interfaces.py
try:
transaction_fee = self.payload['gas'] * self.payload['gasPrice']
except KeyError:
return self.default
Copy link
Member

Choose a reason for hiding this comment

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

I guess this commit never made it in because #2369 wasn't merged... @KPrasch cherry-picked it

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, and I actually changed it a bit more since then.

derekpierre
derekpierre reviewed Oct 16, 2020
View reviewed changes
nucypher/network/nodes.py
@@ -1359,7 +1359,7 @@ def node_details(node):
"last_seen": last_seen,
"fleet_state": node.fleet_state_checksum or 'unknown',
"fleet_state_icon": fleet_icon,
"domain": node.learning_domain,
"domain": node.serving_domain,
Copy link
Member

Choose a reason for hiding this comment

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

This will help with the monitor - the other problematic line was

nucypher/nucypher/network/nodes.py

Line 353 in ad6b6bd

node_domain = node.domain.decode('utf-8')
.

Copy link
Member

Choose a reason for hiding this comment

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

My current workaround for the monitor - derekpierre@ad2e2d0

Copy link
Member Author

Choose a reason for hiding this comment

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

That's line is tricky, because I know that it doesn't fail in other contexts (e.g., when it's a file-based node storage). Not sure how to proceed here until #2356 is merged. What about an ugly try block?

Copy link
Member

Choose a reason for hiding this comment

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

I'm good with the ugly for now - with a todo to fix properly later. Just gets us moving before a more comprehensive solution.

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you think of this for line 353?

            try:  # Workaround until #2356 is fixed
                node_domain = node.domain.decode('utf-8')
            except:
                node_domain = node.serving_domain

Copy link
Member

Choose a reason for hiding this comment

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

Looks good. We can be specific about the AttributeError - but looks good to me anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if it's an attribute error or a BytestringSplitter error. In any case, it's ugly af and we need to fix #2356 soon 😅

Copy link
Member

Choose a reason for hiding this comment

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

In a follow-up PR we can collapse learning_domain and serving_domain into domain.

derekpierre
derekpierre approved these changes Oct 16, 2020
View reviewed changes
Copy link
Member

@derekpierre derekpierre left a comment

Choose a reason for hiding this comment

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

🎸

fjarri
fjarri reviewed Oct 16, 2020
View reviewed changes
nucypher/blockchain/eth/interfaces.py
transaction_fee = self.payload['gas'] * self.payload['gasPrice']
except KeyError:
return self.default
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think else is necessary here. You're returning from the exception branch anyway.

@vepkenez
Copy link
Contributor

vepkenez commented Oct 16, 2020 •
edited

I will not approve unless this PR has a fix for this:

	  File "/usr/local/lib/python3.7/site-packages/nucypher/network/nodes.py", line 1370, in abridged_node_details
	    known = self.known_nodes_details()
	  File "/usr/local/lib/python3.7/site-packages/nucypher/network/nodes.py", line 1334, in known_nodes_details
	    abridged_nodes[checksum_address] = self.node_details(node=node)
	  File "/usr/local/lib/python3.7/site-packages/nucypher/network/nodes.py", line 1362, in node_details
	    "domain": node.learning_domain,
	AttributeError: 'Ursula' object has no attribute 'learning_domain'

--edit
Oh and it does!

@KPrasch KPrasch merged commit fa1cadd into nucypher:main Oct 16, 2020
@KPrasch KPrasch mentioned this pull request Oct 20, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fjarri fjarri fjarri left review comments

@vepkenez vepkenez vepkenez approved these changes

@KPrasch KPrasch KPrasch approved these changes

@tuxxy tuxxy tuxxy approved these changes

@derekpierre derekpierre derekpierre approved these changes

@jMyles jMyles Awaiting requested review from jMyles

@vzotova vzotova Awaiting requested review from vzotova

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@cygnusv @vepkenez @fjarri @KPrasch @tuxxy @derekpierre