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

Remove deprecated constant from the crypto contract #1447

Closed
3 of 5 tasks
tarakby opened this issue Feb 25, 2022 · 8 comments · Fixed by #2984
Closed
3 of 5 tasks

Remove deprecated constant from the crypto contract #1447

tarakby opened this issue Feb 25, 2022 · 8 comments · Fixed by #2984
Assignees
Labels
Feature Language Breaking Change Breaks Cadence contracts deployed on Mainnet

Comments

@tarakby
Copy link
Contributor

tarakby commented Feb 25, 2022

Context

FVM updated the requirement about user signature tags to accept all tags (onflow/flow-go#2171). The user tag is deprecated (not a protocol constant anymore) and should not be used.
It is still hardcoded in the crypto contract to verify signatures (https://github.com/onflow/cadence/blob/49c7bc8/runtime/stdlib/contracts/crypto.cdc#L155)

The crypto contract API could also take a tag parameter, which would be a breaking change.

The contract is currently part of the Cadence code base. We can take this opportunity to move the contract out of Cadence and deploy it on both testnet and mainnet. This allows future contract updates to happen without a spork.

Definition of done:

  • add a new tag parameter for the verify function and use it to verify each signature.
  • remove the flow user tag from the contract
  • update the contract tests
  • (optional) move the contract and tests outside of Cadence
  • (optional) deploy the new contract to testnet and mainnet
@j1010001
Copy link
Member

j1010001 commented Jun 3, 2022

Hi @tarakby , is this still relevant ?

@tarakby
Copy link
Contributor Author

tarakby commented Jun 6, 2022

Hi @j1010001 , yep

@j1010001 j1010001 added the Language Breaking Change Breaks Cadence contracts deployed on Mainnet label Jan 13, 2023
@tarakby
Copy link
Contributor Author

tarakby commented Jul 14, 2023

Hi @j1010001 , this is a very long technical debt. I'm putting it on your radar again

@tarakby tarakby changed the title Update KeyList.verify to accept a tag parameter Remove deprecated constant from crypto contract Jul 14, 2023
@tarakby tarakby changed the title Remove deprecated constant from crypto contract Remove deprecated constant from the crypto contract Jul 14, 2023
@turbolent
Copy link
Member

@tarakby Could you please add some reasoning why this should be done?

Given it is a breaking change, we would need to include it in the Stable Cadence release. So we need to get approval (FLIP) for it.

@tarakby
Copy link
Contributor Author

tarakby commented Aug 10, 2023

As the linked PR mentions, the Flow protocol used to define two signature tags (one for transactions, one for all user applications). This Cadence contract used the second tag which was correct at that time.
In February 2022, we decided that the Flow protocol should only define the transaction tag, it should deprecate the user tag, and let applications choose their own tags. That's when I created this issue and there were updates to other tools.
The Cadence crypto contract didn't get updated and some applications couldn't use it (for example FCL needed this code but ended up deploying its own contract use a different tag).

I believe a protocol FLIP was needed in Feb 2022 to deprecate the user tag, but updating Cadence is only a technical debt. I don't believe a FLIP is needed at this point because the decision was already taken and implemented by other tools.

I also remember you mentioned that this contract should not be part of Cadence, so maybe stable Cadence could decide to not include it at all?

@turbolent
Copy link
Member

turbolent commented Dec 19, 2023

Moving the Crypto contract out of Cadence will require:

  • Moving the contract to the Flow core contracts repository (https://github.com/onflow/flow-core-contracts) (tiny)
  • Move tests (small)
  • Deploy the contract on all chains (medium)
  • Add state migration which migrates existing stored values to the respective new deployed contract (large)

No additional work in the contract itself is required, we had already previously refactored it to be deployable like any other contract

@turbolent
Copy link
Member

@tarakby Can you please open a separate issue for moving the Crypto contract out of Cadence?

@tarakby
Copy link
Contributor Author

tarakby commented Feb 26, 2024

I created #3135 for the contract migration task. Not sure if it's a breaking change though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Language Breaking Change Breaks Cadence contracts deployed on Mainnet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants