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

read-msg recoverable under some key #1223

Merged
merged 4 commits into from
May 10, 2023
Merged

read-msg recoverable under some key #1223

merged 4 commits into from
May 10, 2023

Conversation

jmcardon
Copy link
Member

@jmcardon jmcardon commented May 9, 2023

PR checklist:

  • Test coverage for the proposed changes
  • PR description contains example output from repl interaction or a snippet from unit test output
"Setting transaction data"
["DisablePact47"]
"Expect failure: success: read-integer fails on non-existent key"
"Expect failure: success: read-string fails on non-existent key"
"Expect failure: success: read-keyset fails on non-existent key"
"Expect failure: success: read-decimal fails on non-existent key"
"Expect failure: success: read-msg fails on non-existent key"
"Commit Tx 4"
"Begin Tx 5"
"Setting transaction data"
[]
"Expect: success: read-integer fails on non-existent key but is recoverable"
"Expect: success: read-string fails on non-existent key but is recoverable"
"Expect: success: read-keyset fails on non-existent key but is recoverable"
"Expect: success: read-decimal fails on non-existent key but is recoverable"
"Expect: success: read-msg fails on non-existent key but is recoverable"
"Commit Tx 5"

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

  • Confirm replay/back compat
    Forked. Needs replay
  • Benchmark regressions
    No point in this PR.

@jmcardon jmcardon requested review from jwiegley and emilypi as code owners May 9, 2023 16:18
CHANGELOG.md Outdated Show resolved Hide resolved
@sirlensalot
Copy link
Contributor

Some comments via the new template:

PR description contains example output from repl interaction or a snippet from unit test output

Didn't see this in the description

Confirm replay/back compat
Forked. Does not need replay

This actually means it does need a replay as any fork could have a bug.

Copy link
Contributor

@jwiegley jwiegley left a comment

Choose a reason for hiding this comment

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

This looks very straightforward to me. Thank you for the tests!

@jmcardon
Copy link
Member Author

jmcardon commented May 9, 2023

Some comments via the new template:

PR description contains example output from repl interaction or a snippet from unit test output

Didn't see this in the description

Fixed, sorry I thought i pasted it

Confirm replay/back compat
Forked. Does not need replay

This actually means it does need a replay as any fork could have a bug.

Ah, oops. Yeah it is a semantic change

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.

Needs replay/justification to not replay + IT and I'll approve

Comment on lines 89 to 93
isExecutionFlagSet FlagDisablePact47 >>= \case
True ->
evalError' i $ "No such key in message: " <> pretty k
False ->
failTx' i $ "No such key in message: " <> pretty k
Copy link
Member

@emilypi emilypi May 9, 2023

Choose a reason for hiding this comment

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

Optional suggestion:

Suggested change
isExecutionFlagSet FlagDisablePact47 >>= \case
True ->
evalError' i $ "No such key in message: " <> pretty k
False ->
failTx' i $ "No such key in message: " <> pretty k
ifExecutionFlagSet FlagDisablePact47
(evalError' i $ "No such key in message: " <> pretty k)
(failTx' i $ "No such key in message: " <> pretty k)

@emilypi emilypi mentioned this pull request May 10, 2023
4 tasks
@jmcardon
Copy link
Member Author

@jmcardon jmcardon merged commit 4f51eec into master May 10, 2023
@emilypi emilypi deleted the jose/read-x branch May 10, 2023 20:26
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.

4 participants