-
Notifications
You must be signed in to change notification settings - Fork 19
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
Added init_running contract function #536
Merged
Merged
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
45c2d16
added init_running contract function
volovyks 8cd84af
Merge branch 'develop' into serhii/init-running
volovyks e8e4115
Merge branch 'develop' into serhii/init-running
volovyks b09385f
init_running should initialize counter
volovyks 817a303
Merge branch 'develop' into serhii/init-running
volovyks File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
what happens to the pending requests in old contract?
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.
We should not move old requests to the new contract since they will be outdated while you transfer them. Nothing bad will happen if nodes try to push signatures for a non-existent payload.
Votes are not transferred either.
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.
how would they be outdated? I think it's still fair to maintain these requests though otherwise clients will not be able to tell where their signature went (especially when the NEP lands). We should at the very least report back an error somehow
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.
They will get an error from the old contract. These requests are short-lived and contract migration happens rarely. Also, nodes will need to change the contract manually and that will take more than 8 seconds. We will have some downtime.
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.
We may have a more complex design, like a voting mechanism for the "next" contract. But I do not think that is necessary now. This function is mostly for emergencies.
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.
About moving requests from contract to contract. That is not possible with the current design. We are targeting Sync calls only. Each request must be deleted from the queue in the same sign it was added with. Manually added requests will stay there forever.
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.
yeah this is fine for now as a emergency lever, but ideally we shouldn't be changing contracts. For the error, I'm talking about not just contract-level errors, but applications that are currently relying on indexer to check whether or not a signature has come in. Those applications will forever wait, which isn't good
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.
With the current design, nobody will wait for a signature in an async way. Even if we change the design in the future, such applications should have their timeout.
I agree that we should avoid moving from contract to contract since that is a breaking change for all the clients.
We should be able to handle all the breaking changes in the same contract without moving to a new one.
When adding this function, I was mostly thinking about situations, when we lock the contract to make sure it can not be updated. And then if needed, we can move to another contract using this function.
Should we introduce another contract that will redirect sign calls to the current version of the real MPC contract?
We will control such a contract by setting the current MPC contract ID each time we change it. Loosing this key will not introduce any security threats.
At the same time, all versions of the MPC contract will be locked to prevent all sorts of attacks with stolen contract keys.