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.

Sign up for GitHub

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

Jump to bottom

[fwd port] Fix local contract upgrade #20743

Merged
merged 5 commits into from
Feb 11, 2025
Merged

[fwd port] Fix local contract upgrade #20743

merged 5 commits into from
Feb 11, 2025

Conversation

paulbrauner-da
Copy link
Contributor

@paulbrauner-da paulbrauner-da commented Feb 7, 2025 •
edited
Loading

Port of #20296. SBuiltinFun is so different from its 2.x version that I ported the change manually in commit e7e307d.

Commit cec59bd is a cherry-pick of the original PR, except for the SBuiltin part, and I keep some test cases that the original PR deleted because we'll want them in a dynamically typed world.

Commit c6848e7 makes these test cases that I kept pass.

Part of https://github.com/DACH-NY/canton/issues/23825.

paulbrauner-da and others added 4 commits February 7, 2025 18:26
@paulbrauner-da
manual port of the SBuiltinFun change of pull/20296
e7e307d
@paulbrauner-da @remyhaemmerle-da
Fix local contract upgrade (#20296)
cec59bd 
* Add metadata test for local contracts, fix logic

* factorize the code

* fix tests but also uncovered a problem with interfaces

* use toInterfaceContractId in daml-script tests

* fix typo in pretty-printer

* add positive tests

* fix exercise by interface

* always perform the upgrade validation check in the engine

* simplify error reporting

* ensure package is loaded before importing global contract

* simplify check code even more

* allow losing observers that are also signatories

* Apply suggestion

Co-authored-by: Remy <remy.haemmerle@daml.com>

---------

Co-authored-by: Remy <remy.haemmerle@daml.com>
@paulbrauner-da
fix UpgradeTest
c6848e7
@paulbrauner-da
format
62f7cd9
@samuel-williams-da
Copy link
Contributor

Seems the tests you ported don't match the current state of upgrades errors in trySubmit. Carl hasn't ported some changes yet so they're still seen as a dev error

@paulbrauner-da
Copy link
Contributor Author

Seems the tests you ported don't match the current state of upgrades errors in trySubmit. Carl hasn't ported some changes yet so they're still seen as a dev error

Disabled the failing tests with a TODO and an explanation.

@paulbrauner-da paulbrauner-da mentioned this pull request Feb 11, 2025
Merged
samuel-williams-da
samuel-williams-da approved these changes Feb 11, 2025
View reviewed changes
Copy link
Contributor

@samuel-williams-da samuel-williams-da left a comment

Choose a reason for hiding this comment

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

Thanks Paul! Looks good :)

@samuel-williams-da
Copy link
Contributor

You may want to mention this ticket on the spreadsheet entry for key normalization, so we don't forget it :)

@paulbrauner-da paulbrauner-da merged commit 3c30b11 into main Feb 11, 2025
15 checks passed
@paulbrauner-da paulbrauner-da deleted the pb/port-local-contract-upgrade branch February 11, 2025 12:50
@paulbrauner-da
Copy link
Contributor Author

You may want to mention this ticket on the spreadsheet entry for key normalization, so we don't forget it :)

I mention the corresponding work items in the SCU project board in the TODOs and I'll make sure we don't have any remaining TODOs mentioning these issues before closing them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samuel-williams-da samuel-williams-da samuel-williams-da approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@paulbrauner-da @samuel-williams-da