Consider preventing db modification in the REPL when a function call fails in (expect-failure)

[thomashoneyman]

In the REPL I've noticed that if a function fails within (expect-failure) but writes to the database along the way then it isn't rolled back. Any writes that happened prior to the failure mutate the database. Here's a small reproduction:

; repro.repl
(begin-tx)
(module test-mod GOV
  (defcap GOV () true)

  (defschema row balance:decimal)
  (deftable balances:{row})

  (defun set-balance (amount:decimal)
    (write balances "row" { "balance": amount })
    (enforce (> amount 0.0) "Amount must be positive"))

  (defun get-balance ()
    (read balances "row"))
)
(create-table balances)
(commit-tx)

(begin-tx)
(test-mod.set-balance 100.0)
(print (test-mod.get-balance))
(commit-tx)

(begin-tx)
(expect-failure "" (test-mod.set-balance 0.0))
(commit-tx)

(begin-tx)
(print (test-mod.get-balance))
(commit-tx)

In this code, the second set-balance call fails as indicated by the (expect-failure). But the output reflects that the database was written anyway:

λ pact repro.repl
{"balance": 100.0}
{"balance": 0.0}
Load successful

If I replace the (commit-tx) after (expect-failure) with (rollback-tx) then we're back to the expected output. Also, if I swap the order of the enforce / write statements in (set-balance) then we're also back to the expected output. (In the real code that spurred this question it isn't possible to replace the order of statements).

Is this expected? In my mental model, a failed function call rolls back any db modification made within its body, but that's not what I'm seeing here. There is obviously no (expect-failure) on-chain, so presumably this would actually fail in a transaction and the db modification would never happen in the first place. But in the REPL environment I was surprised that a failed function call will still cause a database modification.

I do expect some measure of weirdness from using (expect-failure) at all, as it "passes" a function that would otherwise fail the transaction altogether, so maybe this isn't an issue at all. I thought I'd bring it to y'all's attention, though, since it was a surprise to me.

[jmcardon]

Hey @thomashoneyman thanks for filing the issue!

We're looking into it. I tried working on a POC yesterday, but our complicated bit is that our pactdb abstraction lacks savepoints to rollback to. Another alternative is to get the delta of the TxLogs from before the expression evaluation to post-expression eval, and apply the reverse action on each one, but we also do not expose a way to simply delete entries.

I think this will require a special repl pact db abstraction, with either savepoints or some form of key deletion, that's only accessible to the repl. We will look into it, since I definitely see your point.

[jmcardon]

I am unsure whether I want to keep this issue open.

This kind of change might break a lot of people's tests. Moreover, you can act as the person to rollback the db changes yourself via

(begin-tx)
(expect-failure ...)
(commit-tx)

It's a bit of weirdness, I understand, but also the repl doesn't understand "auto-rollback" on failure either, it's a bit more bare bones than that, and I do worry about breaking a downstream user's tests