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

fix!: avoid deploying a contract to an account with existing state #802

Merged
merged 13 commits into from
Mar 25, 2022

Conversation

esaminu
Copy link
Contributor

@esaminu esaminu commented Feb 15, 2022

This PR addresses near/near-wallet#2261 and avoids bricking accounts with arbitrary contract state left over by doing the following:

  • The disable 2fa procedure will clear all contract state after itself in 2 atomic steps:

    1. Clear all requests by calling delete_request to ensure that the rpc view_state call needed to use the state cleanup contract's clean method does not throw a TOO_LARGE_CONTRACT_STATE error due to the state size limit.
    2. Deploy of the state cleanup contract passed as an argument and call the clean method in the same transaction as the standard key deletion and conversion steps and finally deploy of the 'empty' contract that is passed as an argument.
  • The enable 2fa procedure will submit a transaction that will always fail designed to identify the current state of the contract it is interacting with. The transaction has 2 actions, one that deploys the multisig contract and the second calls delete_request with the request_id argument set to u32::MAX.

    • If the transaction throws a 'No such request' error, we know that the state in the contract is valid multisig state and we proceed with the enable 2fa operation.
    • If the transaction throws a 'Multisig contract should be initialized' error, we know that we are dealing with a contract that has no state so we proceed with the enable 2fa operation adding a new function call at the end of the action batch.
    • If the transaction throws a 'Cannot deserialize the contract state' error then we know the contract we are dealing with has arbitrary contract state and we abort the operation throwing a contractHasExistingStateError.
    • If the transaction throws any other error, then we abort, throwing that error.

This was tested with different combinations of contract states and code_hashes here

@frol
Copy link
Collaborator

frol commented Feb 17, 2022

This is currently blocked due to view_state requests having a limit and not being paginated.

@bowenwang1996 Is there a chance that fetching a list of state keys can be faster than fetching the key-values?

@bowenwang1996
Copy link
Contributor

This is currently blocked due to view_state requests having a limit and not being paginated.

@bowenwang1996 Is there a chance that fetching a list of state keys can be faster than fetching the key-values?

I don't think so, at least with the current implementation

@esaminu esaminu marked this pull request as ready for review February 23, 2022 21:19
@esaminu esaminu requested a review from vgrichina as a code owner February 23, 2022 21:19
Copy link
Contributor

@MaximusHaximus MaximusHaximus left a comment

Choose a reason for hiding this comment

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

Nice work on identifying all of the underlying behaviours we've been encountering with this logic 💖 Added a few questions/concerns in-line...

src/account_multisig.ts Outdated Show resolved Hide resolved
src/account_multisig.ts Outdated Show resolved Hide resolved
src/account_multisig.ts Outdated Show resolved Hide resolved
src/account_multisig.ts Outdated Show resolved Hide resolved
src/account_multisig.ts Outdated Show resolved Hide resolved
src/account_multisig.ts Show resolved Hide resolved
src/account_multisig.ts Show resolved Hide resolved
src/account_multisig.ts Outdated Show resolved Hide resolved
@esaminu esaminu requested a review from MaximusHaximus March 17, 2022 00:22
Copy link
Contributor

@MaximusHaximus MaximusHaximus left a comment

Choose a reason for hiding this comment

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

We're just about there - just a few things in-line and then we can get this landed 👍 <3

src/account_multisig.ts Outdated Show resolved Hide resolved
src/account_multisig.ts Outdated Show resolved Hide resolved
src/account_multisig.ts Show resolved Hide resolved
src/account_multisig.ts Outdated Show resolved Hide resolved
Co-authored-by: Daryl Collins <daryl@darylcollins.com>
@esaminu esaminu force-pushed the contract-deploy-validate-existing-state branch from 9236623 to fedd3d5 Compare March 25, 2022 18:11
@esaminu esaminu requested a review from MaximusHaximus March 25, 2022 18:26
Copy link
Contributor

@MaximusHaximus MaximusHaximus left a comment

Choose a reason for hiding this comment

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

👍 nice work! Excited to get this landed :)

@MaximusHaximus MaximusHaximus changed the title fix: avoid deploying a contract to an account with existing state fix!: avoid deploying a contract to an account with existing state Mar 25, 2022
@MaximusHaximus MaximusHaximus merged commit 4163807 into master Mar 25, 2022
@MaximusHaximus MaximusHaximus deleted the contract-deploy-validate-existing-state branch March 25, 2022 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants