-
Notifications
You must be signed in to change notification settings - Fork 144
proxy: refactor #3709
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
base: master
Are you sure you want to change the base?
proxy: refactor #3709
Conversation
2a1cf98
to
281e715
Compare
I'm not sure why it's not triggering in the CI, but when I
which appears to indeed be the case:
|
let providerId = | ||
try: | ||
await engine.backend.eth_chainId() | ||
except CatchableError as e: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nimbus-eth1/nimbus_verified_proxy/nimbus_verified_proxy.nim(48, 30) Hint: 'e' is declared but not used [XDeclaredButNotUsed]
Can just omit the as e
, e.g.
except CatchableError:
Separately, there are a lot of CatchableError
references throughout, but that can be addressed via a separate comment. As far as this specific one goes, it's just the XDeclaredButNotUsed
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that said, almost every time you do catch CatchableError in async code, you must also propagate CancelledError, ie catch and reraise it: https://www.youtube.com/watch?v=1bBVQaAD_jI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you intend to attach some other link to this comment? or is there a metaphorical meaning to the video?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
606c8db @arnetheduck returning cancelled error now. changed it wherever CatchableError
was caught but no other error was propagated.
42cd4e8
to
606c8db
Compare
Co-authored-by: kdeme <[email protected]>
2835879
to
5b3fa62
Compare
Verified proxy consists fo two components the verifying algorithms and the proxy. This PR logically splits the verification engine from the proxy. The proxy is further broken down into seperate client and server implementations that connect to the verification engine as backend and frontend resp.