-
Notifications
You must be signed in to change notification settings - Fork 107
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
Conversation
@rsoeldner looks good from here. For something like this, I'd feel most comfortable with a full mainnet replay |
@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: |
There was a problem hiding this 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.
I'm comfortable with this change, and here's my reasoning below:
The conclusion is that: |
(QName (QualifiedName (ModuleName mn mNs) fn i)) | ||
| not flagPact48Disabled | ||
&& mn == _mnName (_mName mdef) | ||
&& isNsMatch -> resolveBareName memo (BareName fn i) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Great job
There was a problem hiding this 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Great job
Dismissing due to the fact that we found new problems.
Signed-off-by: Robert Soeldner <r.soeldner@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More test review
This PR aims to fix the module-redeployment issue indicated by the upcoming example:
Here, the full qualified name
free.m.test
will load the previous version of the contract.PR checklist:
cabal run tests
. If they pass locally, docs are generated.pact -t
), make sure pact-lsp is in sync.Additionally, please justify why you should or should not do the following:
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