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 name resolution within module redeploy #1235

Merged
merged 20 commits into from
Jun 27, 2023
Merged

Conversation

rsoeldner
Copy link
Member

@rsoeldner rsoeldner commented May 29, 2023

This PR aims to fix the module-redeployment issue indicated by the upcoming example:

(begin-tx)
(module ezfree G (defcap G () true) (defun ALLOW () true))
(define-namespace 'free (create-user-guard (ALLOW)) (create-user-guard (ALLOW)))
(commit-tx)

(begin-tx)
(namespace 'free)
(module m g
  (defcap g () true)

  (defcap test ()
    (enforce false "boom"))

  (defun f ()
    (with-capability (test)
      1))
  )
(commit-tx)

(begin-tx)
(namespace 'free)
(module m g
  (defcap g () true)
  (defcap test ()
    true)
  (defun f ()
    (with-capability (free.m.test)
      1))
  )

(f)

(commit-tx)

Here, the full qualified name free.m.test will load the previous version of the contract.

PR checklist:

  • Test coverage for the proposed changes
  • PR description contains example output from repl interaction or a snippet from unit test output
  • Documentation has been updated if new natives or FV properties have been added. To generate new documentation, issue cabal run tests. If they pass locally, docs are generated.
  • Any changes that could be relevant to users have been recorded in the changelog
  • In case of changes to the Pact trace output (pact -t), make sure pact-lsp is in sync.

Additionally, please justify why you should or should not do the following:

  • Confirm replay/back compat
  • Benchmark regressions
  • (For Kadena engineers) Run integration-tests against a Chainweb built with this version of Pact

Integration test results: https://github.com/kadena-io/integration-tests/actions/runs/5111894752

Mainnet reply results: https://github.com/kadena-io/integration-tests/actions/runs/5115086788/jobs/9196019646

@rsoeldner rsoeldner requested a review from jmcardon May 29, 2023 11:50
@rsoeldner rsoeldner marked this pull request as ready for review May 30, 2023 14:42
src/Pact/Eval.hs Outdated Show resolved Hide resolved
src/Pact/Eval.hs Outdated Show resolved Hide resolved
tests/pact/modrefs.repl Outdated Show resolved Hide resolved
tests/pact/modrefs.repl Outdated Show resolved Hide resolved
tests/pact/modrefs.repl Outdated Show resolved Hide resolved
tests/pact/modrefs.repl Outdated Show resolved Hide resolved
src/Pact/Eval.hs Outdated Show resolved Hide resolved
src/Pact/Eval.hs Outdated Show resolved Hide resolved
src/Pact/Eval.hs Outdated Show resolved Hide resolved
src/Pact/Eval.hs Outdated Show resolved Hide resolved
src/Pact/Eval.hs Outdated Show resolved Hide resolved
tests/pact/modrefs.repl Outdated Show resolved Hide resolved
tests/pact/fqns.repl Outdated Show resolved Hide resolved
src/Pact/Eval.hs Outdated Show resolved Hide resolved
tests/pact/fqns.repl Outdated Show resolved Hide resolved
tests/pact/fqns.repl Show resolved Hide resolved
emilypi
emilypi previously approved these changes Jun 8, 2023
@emilypi emilypi dismissed their stale review June 8, 2023 21:47

We need replay!

@emilypi
Copy link
Member

emilypi commented Jun 8, 2023

@rsoeldner looks good from here. For something like this, I'd feel most comfortable with a full mainnet replay

@rsoeldner
Copy link
Member Author

rsoeldner commented Jun 9, 2023

@emilypi I did last week https://github.com/kadena-io/integration-tests/actions/runs/5115086788

But also kicked off a new one with the latest changes. I will report the results once it's done

New Tests:

emilypi
emilypi previously approved these changes Jun 12, 2023
Copy link
Member

@emilypi emilypi left a comment

Choose a reason for hiding this comment

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

Thanks for the update @rsoeldner. IT and replay passed, so approved.

@edmundnoble
Copy link
Contributor

I'm comfortable with this change, and here's my reasoning below:

  1. Modules can only be affected by this change when they're redeployed, from what I understand.
  2. If a module, let's say version 2 of that module, depends on the old behavior, it was implicitly relying on the fact that its qualified self-references referred to version 1, and not version 2.
  3. When the module is redeployed (now as version 3) without this change to name resolution, its qualified self-references begin to refer to version 2. The module will only keep work if version 3 is fine referring to version 2 through its qualified self-references.
  4. When the module is redeployed with this change to name resolution, its qualified self-references begin to refer to version 3. The module will only work if version 3 is fine referring to version 3 through its qualified self-references.

The conclusion is that:
5. The only new breakage that we can introduce with this change requires that version 2 works calling into version 1 but not 2, and version 3 works calling into version 2 but not 3. Seems like an incredibly uncommon event. I would expect that if version 2 works calling into version 1 but not 2, this is because of some new bug with the code called into in version 2. If that bug was fixed by version 3, then there is no issue with version 3 calling into itself. If that wasn't fixed by version 3, then calling into version 2 instead will still be a problem, meaning that there is a problem regardless of this name resolution change.

(QName (QualifiedName (ModuleName mn mNs) fn i))
| not flagPact48Disabled
&& mn == _mnName (_mName mdef)
&& isNsMatch -> resolveBareName memo (BareName fn i)
Copy link
Member Author

Choose a reason for hiding this comment

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

Compared to 4ca5571, we check if the symbol is prefixed by a namespace, if so we check against the module.

Copy link
Member

Choose a reason for hiding this comment

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

👍 Great job

@rsoeldner
Copy link
Member Author

rsoeldner commented Jun 21, 2023

Copy link
Member

@jmcardon jmcardon left a comment

Choose a reason for hiding this comment

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

Haskell code is fine, just missing 1 test then can approve this PR.

(QName (QualifiedName (ModuleName mn mNs) fn i))
| not flagPact48Disabled
&& mn == _mnName (_mName mdef)
&& isNsMatch -> resolveBareName memo (BareName fn i)
Copy link
Member

Choose a reason for hiding this comment

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

👍 Great job

tests/pact/fqns.repl Show resolved Hide resolved
tests/pact/fqns.repl Outdated Show resolved Hide resolved
tests/pact/fqns.repl Outdated Show resolved Hide resolved
tests/pact/fqns.repl Outdated Show resolved Hide resolved
tests/pact/fqns.repl Outdated Show resolved Hide resolved
tests/pact/fqns.repl Outdated Show resolved Hide resolved
@emilypi emilypi dismissed their stale review June 23, 2023 19:00

Dismissing due to the fact that we found new problems.

@jmcardon jmcardon dismissed their stale review June 23, 2023 19:05

Problems were fixed

tests/pact/fqns.repl Outdated Show resolved Hide resolved
Signed-off-by: Robert Soeldner <r.soeldner@gmail.com>
Copy link
Contributor

@sirlensalot sirlensalot left a comment

Choose a reason for hiding this comment

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

More test review

tests/pact/fqns.repl Outdated Show resolved Hide resolved
tests/pact/fqns.repl Outdated Show resolved Hide resolved
tests/pact/fqns.repl Outdated Show resolved Hide resolved
tests/pact/fqns.repl Outdated Show resolved Hide resolved
tests/pact/fqns.repl Outdated Show resolved Hide resolved
tests/pact/fqns.repl Outdated Show resolved Hide resolved
tests/pact/fqns.repl Outdated Show resolved Hide resolved
@emilypi emilypi merged commit 640e39d into master Jun 27, 2023
6 checks passed
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.

None yet

6 participants