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 simple script distinction #4763

Merged
merged 2 commits into from Mar 8, 2023

Conversation

Jimbo4350
Copy link
Contributor

@Jimbo4350 Jimbo4350 commented Jan 9, 2023

Resolves: #4261

  • Timelock scripts are a superset of MultiSig scripts in that they can also express validation in terms of before or after a particular slot.

  • In cardano-ledger the serialization of Timelock and MultiSig scripts are identical.

Due to these properties, it is not necessary to distinguish between MultiSig and Timelock scripts. We can default to Timelock scripts without issue.

@Jimbo4350 Jimbo4350 force-pushed the jordan/remove-simple-script-distinction branch from 63921d8 to 060cc49 Compare January 10, 2023 19:53
@Jimbo4350 Jimbo4350 marked this pull request as ready for review January 10, 2023 19:53
@Jimbo4350 Jimbo4350 force-pushed the jordan/remove-simple-script-distinction branch 2 times, most recently from 6edf77c to 73bdb37 Compare January 10, 2023 20:28
go (RequireSignature (PaymentKeyHash kh))
= Shelley.RequireSignature (Shelley.coerceKeyRole kh)
go (RequireAllOf s) = Shelley.RequireAllOf (map go s)
go (RequireAnyOf s) = Shelley.RequireAnyOf (map go s)
go (RequireMOf m s) = Shelley.RequireMOf m (map go s)
go timelock = error $ "toShelleyMultiSig: " <> show timelock <>
" not supported in MultiSig scripts."
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if it might be better not to error here.

Using error from the CLI is okay, but error would cause an application that uses the API to exit.

Copy link
Contributor Author

@Jimbo4350 Jimbo4350 Feb 27, 2023

Choose a reason for hiding this comment

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

So I opted to return an Either as you suggested for toShelleyMultiSig because this is a function a consumer of the api is likely to use and potentially shoot themselves in the foot. However toShelleyMultiSig is called in: toShelleyScript :: ScriptInEra era -> Ledger.Script (ShelleyLedgerEra era). I opted to error here instead because this will error if someone is running a Shelley era network and tries to use a Timelock. I think the odds of this is extremely low and not worth polluting the code further with an Either. Lmk what you think.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think let this pass for now given the low probably of it being a problem, but we should also create a ticket to remove calls to error from cardano-api as a separate piece of work to be done after the next release.

@Jimbo4350 Jimbo4350 force-pushed the jordan/remove-simple-script-distinction branch 3 times, most recently from 06238b7 to 0e5d91e Compare February 27, 2023 19:01
@newhoggy newhoggy requested review from newhoggy and removed request for LudvikGalois March 1, 2023 01:53
@Jimbo4350 Jimbo4350 force-pushed the jordan/remove-simple-script-distinction branch 3 times, most recently from 75d9ed7 to b8aa9dc Compare March 6, 2023 16:46
@Jimbo4350 Jimbo4350 enabled auto-merge March 6, 2023 17:24
@Jimbo4350 Jimbo4350 added this pull request to the merge queue Mar 6, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 6, 2023
@Jimbo4350 Jimbo4350 added this pull request to the merge queue Mar 7, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 7, 2023
@Jimbo4350 Jimbo4350 added this pull request to the merge queue Mar 8, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 8, 2023
@Jimbo4350 Jimbo4350 force-pushed the jordan/remove-simple-script-distinction branch from b8aa9dc to 776a29e Compare March 8, 2023 14:56
@Jimbo4350 Jimbo4350 enabled auto-merge March 8, 2023 14:56
@Jimbo4350 Jimbo4350 added this pull request to the merge queue Mar 8, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 8, 2023
@Jimbo4350 Jimbo4350 added this pull request to the merge queue Mar 8, 2023
Merged via the queue into master with commit 52c8ed6 Mar 8, 2023
20 of 23 checks passed
@iohk-bors iohk-bors bot deleted the jordan/remove-simple-script-distinction branch March 8, 2023 16:09
mkoura added a commit to IntersectMBO/cardano-node-tests that referenced this pull request Mar 8, 2023
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.

[BUG] - SimpleScriptV1 in reference script UTxO is reported as SimpleScriptV2
2 participants