[main-2.x][WIP] IDE Ledger support for upgrades #19013
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
moisesackerman-da
force-pushed
the
akrmn/ide-ledger-upgrades-2.x
branch
from
70f7073
to
bbfa857
Compare
moisesackerman-da
force-pushed
the
akrmn/ide-ledger-upgrades-2.x
branch
from
bbfa857
to
3754e73
Compare
sdk/daml-script/test/src/main/scala/com/daml/lf/engine/script/test/IdeLedgerUpgradesIT.scala
Outdated
Show resolved
Hide resolved
moisesackerman-da
force-pushed
the
akrmn/ide-ledger-upgrades-2.x
branch
from
3754e73
to
c7731be
Compare
…to test ide ledger
moisesackerman-da
force-pushed
the
akrmn/ide-ledger-upgrades-2.x
branch
from
c7731be
to
80057e0
Compare
|
As expected, quite a few of the ported tests fail. Some of them due to using |
|
these are the current test failures (note that |
|
Taking over this PR as Moises is away |
There was a problem hiding this comment.
Current issues:
- IdeLedgerClient gives back LookupErrors (wrapped in SErrorCrash), whereas canton gives generic error message. Fixed temporarily by checking for these errors, but should unify.
- Test suite wasn't enabling upgrades, fixed by adding the flag to
daml-script, but the runner should infer this from LF 1.16 - MultiParticipant tests cannot run in ideLedger. fixed by removing
- ScenarioRunner does not check signatories/observer changes, needs to be updated.
Comment on lines
+209
to
+210
| val enableContractUpgrading = | ||
| languageVersion >= LanguageVersion.Features.packageUpgrades |
There was a problem hiding this comment.
This technically isn't enough. Pending @remyhaemmerle-da decision, daml-script should enable upgrades automatically if the package is LF 1.16 and we're using daml3-script.
At that point, this flag will be removed.
For now, we unfortunately can't easily check if the script is running in daml3-script or not from here, so we resolve the issue by ignoring the flag further down in daml2-script
Comment on lines
+550
to
+556
| _ <- check(signatories, originalSignatories)("signatories") | ||
| recomputedObservers = stakeholders -- signatories | ||
| originalObservers = originalStakeholders -- originalSignatories | ||
| _ <- check(recomputedObservers, originalObservers)("observers") | ||
| _ <- check(keyWithMaintainers, original.contractKeyWithMaintainers)( | ||
| "key value and maintainers" | ||
| ) |
There was a problem hiding this comment.
This uses most of the check that canton uses, except the contract-id is not recalculated. I figured that was overkill and messy for the IDE-Ledger, which has already lived without it for so long. Let me know if this isn't okay
| visibility = ["__pkg__"], | ||
| ) | ||
|
|
||
| da_scala_test( |
There was a problem hiding this comment.
For a future ticket, we should formally port over the grpc tests and integrate ide tests into them, then run this suite on both daml2 and daml3.
For now though, this isn't needed for 2.9.
| @@ -583,8 +585,10 @@ private[lf] class Runner( | |||
| throw new IllegalArgumentException("Couldn't get daml script package name") | |||
| ) match { | |||
| case "daml-script" => | |||
| if (enableContractUpgrading) | |||
| throw new IllegalArgumentException("daml2-script does not support Upgrades natively.") | |||
| // TODO[SW]: Can't check for this now as the Script service needs to run with upgrades enabled, and can't know if its running | |||
There was a problem hiding this comment.
See my first comment
Comment on lines
+885
to
+894
| def newestPackageId(old: PackageId): PackageId = { | ||
| for { | ||
| oldReadable <- getPackageIdReverseMap().get(old) | ||
| oldName = oldReadable.name | ||
| newest <- getPackageIdMap().maxByOption { | ||
| case (k, _) if k.name == oldName => Some(k.version); | ||
| case _ => None; | ||
| } | ||
| } yield newest._2 | ||
| }.getOrElse(old) |
There was a problem hiding this comment.
This is mimicking the default canton package map, but now daml-script supports explicit package preference (internal only) is it worth integrating this now?
| res <- a `trySubmit` exerciseExactCmd cidV1 V1.EnumAdditionalCall | ||
|
|
||
| case res of | ||
| -- IDE Ledger doesn't apply obfuscation, instead the lookup error is wrapped in SCrash |
There was a problem hiding this comment.
This is important, we will fix this when we forward Lookup errors through canton.
| Seq( | ||
| "--ide-ledger", | ||
| s"--script-name=${testCase.name}:main", | ||
| "--enable-contract-upgrading", |
There was a problem hiding this comment.
We'll omit this flag if Remy decides it should be inferred from script type + lf version
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.