-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat: rpc as a task with publishing signature #132
Conversation
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.
The code changes mostly makes sense to me!
But can I just ask, what is the main goal? Is it mainly a refactor to clean up code architecture? Or is it trying to unblock certain tasks and introduce more parallelism? It's not quite clear to me from the PR description, it only describes what the code does, not why.
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.
🚢 from perspective of eth respond() publishing.
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 refactoring should make our lives easier when adding other chains. Other comments are reasonable.
whoops, forgot to mention this in the PR description: it's mostly to unblock new signature from not progressing. Before this PR, signature manager would try to make an RPC call and block the whole entire main loop from progressing. This was especially bad in the case that an error occurs during publishing and we would exponentially retry to publish while still in the signature_manager context, which meant other tasks couldn't progress or more signatures could not be processed. |
?mpc_contract_id, | ||
?account_id, | ||
"initializing protocol with parameters" | ||
); |
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.
Seams useful to me.
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.
it's logged elsewhere
NearClient
andEthClient
for easier handlingGoing to defer moving the other RPC related calls like
join
andreshare
till later because those will require additional changes to the state machine for better handling of errors across a channel