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

Create admin ability to pause locks/burns on Sifnode for Peggy 2.0 deployment #2445

Open
pandaring2you opened this issue Feb 7, 2022 · 28 comments
Assignees
Labels

Comments

@pandaring2you
Copy link
Contributor

Ability to pause locks/burns on Sifnode so that we can deploy peggy 2.0 changes. This functionality won't be needed until mid March.

@Jedi2002 Jedi2002 self-assigned this Mar 2, 2022
@Jedi2002
Copy link
Contributor

Jedi2002 commented Mar 2, 2022

To get some clarity on requirements is the following accurate

Requirements

  • Add a new transaction to pause lock tx and burn tx .
    • Sender can only be admin
  • Add new transaction to unpause lock and burn tx
    • Sender can only be admin

@pandaring2you
Copy link
Contributor Author

yup - basically, we want to stop any imports and exports from happening so that we can deploy the peggy 2.0 changes. if there are any other types of transactions that might affect sifnode while we're deploying, please call that out as well. this is all we could think of when meeting with tim

@Jedi2002
Copy link
Contributor

Jedi2002 commented Mar 3, 2022

Alright that helps,
When we say pause does that mean

  • Reject incoming Lock/Burn transactions ( Easier to implement )
  • Add the transactions to a queue, which will be processed once the transactions are unpaused (Much more difficult to implement)
    @pandaring2you

@Brando753
Copy link
Contributor

To clarify this ask, pauser functionality already exists in the smart contracts. It currently allows a pauser to pause or unpause this system. We would like you to spilt out the roles so that there are two RBAC roles one for pausing and one for unpausing. The theory is that there should be many people allowed to pause the bridge (trusted developers, relayers, witnesses, etc) but only a few could unpause it.

@pandaring2you
Copy link
Contributor Author

@Brando753 - wait are you sure? i thought separating the pauser/unpauser roles on our smart contracts was JP's task: https://app.zenhub.com/workspaces/current-sprint---engineering-615a2e9fe2abd5001befc7f9/issues/sifchain/sifnode/1910

This task we talked about with Tim about pausing imports/exports on the sifnode side.

@timlind
Copy link
Contributor

timlind commented Mar 9, 2022

@Jedi2002 it should be a simple reject system when paused.

In terms of roles I think we'll just have one role that can pause and unpause, we could make them different addresses if need be but is that really necessary?

@pandaring2you pandaring2you assigned iverc and joneskm and unassigned Jedi2002 Mar 24, 2022
@pandaring2you
Copy link
Contributor Author

Would need a different admin role in a separate module.
Est 1 week

@Jedi2002
Copy link
Contributor

Hey @marshsif , I can start on this task if required ,

Requirements

Add a new transaction to pause lock tx and burn tx .
Add new transaction to unpause lock and burn tx

  • Reject all lock and burn transactions when paused .
  • Admin control over these transactions is required .

@marshsif
Copy link
Contributor

OK @Jedi2002 , please do, thanks!

@Jedi2002
Copy link
Contributor

This pr adds the pause functionality
#2665

@Jedi2002
Copy link
Contributor

Hey @pandaring2you Who from peggy team should I tag for pr reviews? The sifnode team is busy with pmtp work rn , it might make more sense for someone from preggy to be reviewing the pr's

@Jedi2002 Jedi2002 assigned Jedi2002 and unassigned iverc and joneskm Apr 28, 2022
@pandaring2you
Copy link
Contributor Author

perhaps @juniuszhou cc @Brando753

@pandaring2you
Copy link
Contributor Author

@smartyalgo is currently reviewing

@pandaring2you
Copy link
Contributor Author

@Jedi2002 - Do you know how much more time it would take to address Michael's comments on the PR and close this out? Would be great to close this out after your work on .45 rather than hand this off to another dev

@pandaring2you
Copy link
Contributor Author

@Jedi2002 syncing with @smartyalgo today to review PR. Likely not an issue

@pandaring2you
Copy link
Contributor Author

From @timlind regarding #2665 : "Thanks. I think we need to get this scheduled for a release before we can merge though."
Hey @timlind - can you explain? Why can't we merge?

@timlind
Copy link
Contributor

timlind commented Jun 6, 2022

@pandaring2you we are in the process of deprecating develop branch and don't want work there that isn't ready to ship. Can this be merged into a feature branch instead?

@pandaring2you
Copy link
Contributor Author

@Brando753 - do you have a recommendation here? can we merge this with future/peggy2?

@pandaring2you
Copy link
Contributor Author

@Jedi2002 - if you have time outside of margin - can you review your PR again and see if we can merge this functionality into trunk? cc @smartyalgo @Brando753

@Jedi2002
Copy link
Contributor

Hey @pandaring2you this work was done a long time ago, Not sure why it was never merged, I do see @smartyalgo approved it . I do not have access to merge to develop.
This needs to be redone now, as the sifnoide code has changed a lot since then, updating the PR and redoing it might almost take the same time .
We should be wrapping up margin pretty soon , at least the V1 , please check with @brianosaurus and assign it to me , I can pick this up after .

@pandaring2you
Copy link
Contributor Author

@smartyalgo to take this on

@smartyalgo
Copy link
Contributor

This should be fine once we rebase the PR onto master. Reached out to him, will see if anything comes up

@pandaring2you
Copy link
Contributor Author

Will get this in tonight for deployment on monday

@pandaring2you
Copy link
Contributor Author

Aiming to have integration test done by EOD tomorrow

@pandaring2you
Copy link
Contributor Author

pandaring2you commented Nov 1, 2022

Will get into v1.0.15beta. JamesG is aiming to release this week. @smartyalgo adding to siftool testing with Jure .

@pandaring2you
Copy link
Contributor Author

Testing should be finished by tomorrow. cc @ghuznee

@pandaring2you
Copy link
Contributor Author

Local testing completed and merged to master. Deployed to testnet. Need to run tests on testnet. Working with Lance and Caner on this for next release

@pandaring2you
Copy link
Contributor Author

@smartyalgo : completed testnet testing, supporting Caner with regression testing. No pending need from him

@smartyalgo - so is this fully tested and ready for release for .15?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants