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

feat: Add recovery pallet in runtime. #1105

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

nakul1010
Copy link
Member

@nakul1010 nakul1010 commented Jun 26, 2023

Closes #736

The Recovery Pallet in Substrate is a social recovery tool that allows users to regain account access through trusted friends' approval. It offers customizable configurations, safeguards, and efficient computational performance.

Key Points

  • Utilizes M-of-N social recovery mechanism with Substrate for account access restoration.
  • Users set a threshold of trusted friends for approving recovery attempts.
  • Configurable parameters include friend selection, approval threshold, and delay period.
  • Requires a deposit for creating a recovery configuration, refundable upon removal, and works based on game theory approach.
  • Provides a secure process for initiating, vouching, and claiming account recovery.
  • Allows updates to recovery configurations based on changing relationships and circumstances.
  • Ensures account security by requiring consensus among trusted friends.
  • Mitigates the risk of key loss or authentication issues, enhancing account security.

For more detailed information, refer to the Substrate documentation on Pallet Recovery.

Note: To enhance the adoption of the pallet among Interlay users, the implementation of a dedicated web app page is essential.

@nakul1010 nakul1010 added the inProgress The PR is still in progress label Jun 26, 2023
@nakul1010 nakul1010 requested review from sander2 and gregdhill June 26, 2023 04:13
@nakul1010 nakul1010 self-assigned this Jun 26, 2023
@nakul1010 nakul1010 added ReviewNeeded The PR can be reviewed by peers and removed inProgress The PR is still in progress labels Jun 26, 2023
Copy link
Member

@gregdhill gregdhill left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this @nakul1010! I've left a few comments on the Interlay runtime but please also consider those for the Kintsugi runtime. It's good that you already ran the benchmarks. In the PR description can you describe a bit about the functionality of the recovery pallet? Specifically how it works and what the benefits are.

parachain/runtime/interlay/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines 1081 to 1086
parameter_types! {
pub const ConfigDepositBase: Balance = 5 * DOLLARS;
pub const FriendDepositFactor: Balance = 50 * CENTS;
pub const MaxFriends: u16 = 9;
pub const RecoveryDeposit: Balance = 5 * DOLLARS;
}
Copy link
Member

Choose a reason for hiding this comment

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

How did you decide on these parameters?

Copy link
Member Author

@nakul1010 nakul1010 Jun 29, 2023

Choose a reason for hiding this comment

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

I had taken reference from the frame config set, but after re-reviewing, I suggest setting one of the following configs defined below.

INTR Config based on KSM Config set

Current Market Price

1 KSM -> 25 USD 
1 INTR -> 0.0159 USD 
pub const ConfigDepositBase: Balance = 8 * Dollars; //0.0000000000125 USD

pub const FriendDepositFactor: Balance = 1 * Dollars; //0.00000000000125 USD

pub const MaxFriends: u16 = 9;

pub const RecoveryDeposit: Balance = 8 * Dollars; //0.0000000000125 USD

INTR Config based on Dollar, this

pub const ConfigDepositBase: Balance = 315 * Dollars; // if 1 INTR -> 0.0159 USD, this is equivalent to 5 Dollars

pub const FriendDepositFactor: Balance = 31 * Dollars; //this is equivalent to 0.5 Dollars

pub const MaxFriends: u16 = 9;

pub const RecoveryDeposit: Balance = 6300 * Dollars; // equivalent to 100 dollars, I recommend setting a larger value for the RecoveryDeposit compared to the ConfigDepositBase. 
// While many para chains set the RecoveryDeposit equal to the ConfigDepositBase, I suggest using a higher amount, 100 Dollars. 
// By increasing the RecoveryDeposit, we can ensure a more robust recovery process and mitigate the risk of unauthorized or malicious recoveries based on game theory.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note: Polkadot yet not have added the recovery pallet into the runtime.

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense to me!

Copy link
Member

Choose a reason for hiding this comment

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

Aren't those values very very high? We require 250 vINTR for governance proposals and these would require a lot more INTR to actually use this feature. So only very few people could use this.

What's the downside of setting this to much lower values so that you could set this up with say max 500 INTR?

Copy link
Member Author

@nakul1010 nakul1010 Jul 4, 2023

Choose a reason for hiding this comment

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

@nud3l, I agree that we can reduce the value of ConfigDepositBase to 100 INTR to make it more accessible. However, it is important to keep the RecoveryDeposit at a sufficiently high level. This is because the RecoveryDeposit amount is refunded if the recovery process is successful.

If we set the RecoveryDeposit value too low, there is a risk that a malicious rescuer could attempt to recover the account, and if the account holder cancels the recovery process, the malicious rescuer would only lose a minimal amount due to the low RecoveryDeposit value.

It's worth noting that the RecoveryDeposit only needs to be reserved if the account recovery process is on and not when setting up the account recovery.

@nakul1010
Copy link
Member Author

Thanks for looking into this @nakul1010! I've left a few comments on the Interlay runtime but please also consider those for the Kintsugi runtime. It's good that you already ran the benchmarks. In the PR description can you describe a bit about the functionality of the recovery pallet? Specifically how it works and what the benefits are.

Done

@nakul1010 nakul1010 requested a review from gregdhill June 29, 2023 13:46
@gregdhill
Copy link
Member

@nud3l @sander2 thoughts on including this pallet?

@nud3l
Copy link
Member

nud3l commented Jul 4, 2023

I'm in favor of including the pallet. @nakul1010 @gregdhill do you know other projects having a UI for this?

I do think the security deposits are too aggressive though for the feature to be useful. Left an extra comment there so we can discuss.

@nakul1010
Copy link
Member Author

I'm in favor of including the pallet. @nakul1010 @gregdhill do you know other projects having a UI for this?

I do think the security deposits are too aggressive though for the feature to be useful. Left an extra comment there so we can discuss.

I couldn't find any team that has a dedicated user interface specifically for this pallet. However, you can find the flow documentation for the process on Polkadot's document website here.

@nud3l
Copy link
Member

nud3l commented Jul 4, 2023

Ok, let me ask if somebody has already implemented a UI for this.

@nakul1010 nakul1010 requested review from nud3l and gregdhill July 10, 2023 18:41
@nakul1010
Copy link
Member Author

@gregdhill updated parameters based on the comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ReviewNeeded The PR can be reviewed by peers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add recovery pallet
3 participants