Make test accounts have normal permissions

[45930]

Repro steps:

Fill in a user private key in this script and attempt to run (private key must be to a funded account on Berkeley). Expected behavior is that the funded account will send the smart contract 1000 base units of Mina, and the contract state (merkle root) will update to reflect the user's deposit. Observed behavior is that the transaction gets built just fine but gets an error on the blockchain for example. The current root hash on the chain is 2273112294.... If that root hash has changed, then that probably means a deposit has worked and the error is resolved.

Script:
https://github.com/pico-labs/coinflip-executor-contract/blob/ee63f90acfb435f06723e08ce00c2792083f8afb/src/berkeleyRun.ts

Deployed Contract:
https://berkeley.minaexplorer.com/wallet/B62qjb2oGVMGiPad6GKTBh6MqgFJ3ExhkfegBuUiynLoyCSbTfM62vW

[mitschabaude]

I get it actually. You're missing the following in your deposit method:

depositUpdate.requireSignature();

therefore, the account update has no authorization on it. The reason it works in tests is that those predefined "test" accounts have their permissions set to not require any authorization.

I think that test behaviour might have been useful at an earlier stage where snarkyjs was less mature, but should be changed now, to not give misleading test results. If you don't mind, I'll change the description of this issue to be about changing permissions on the test accounts

[45930]

Yup, I understand the issue now. I was implicitly expecting #send to require the signature for me, if a signature was required on the sending account. For instance, I was signing the transaction with the player's private key, but I now realize since nothing was asking for a signature, that that was useless.

I guess I would suggest that calling #sign with private keys that are never requested should log a warning or really my preference would be an exception, but maybe that's too strong. And since send is a literal permission on accounts, I think AccountUpdate#send should require a signature implicitly, if a signature is required on that account. Like we've already configured what is needed to send, so why do I have to tell it again? Either or both of those changes would have allowed me to catch this a lot earlier! (In addition to your suggested change of making the test account permissions the same as default accounts on chain)

[mitschabaude]

I think AccountUpdate#send should require a signature implicitly, if a signature is required on that account

that makes sense to me. it's just a lot more involved to require the signature depending on the current account permissions (we're in a SNARK, it's all static). also, we have no way of verifying that the prover provides the correct permissions, there's no precondition for permissions (which doesn't pose a security issue here, as far as I can tell). given all that, I'm not sure making this implicit is worth it.

I'll start by properly documenting all those functions like requireSignature()

[45930]

Yeah that all makes sense but to me it is still kind of weird. #send exists primarily as a convenience method right? To hide account1.balance.add(x); account2.balance.sub(x); or whatever is actually going on. But then the dev 99,999 times out of 100,000 is going to add this requireSignature call which feels out place to, say, a solidity programmer. So I wonder why not just go back and use the more explicit balanceChange API which feels more natural in snarkyjs?

I am seeing this in the source code:

snarkyjs/src/lib/account_update.ts

        // Require signature from the sender accountUpdate
        Authorization.setLazySignature(sender);

That makes it sounds like the intention actually was that update requires a signature implicitly. But then the source code for lazy signature looks more like it's for testing purposes.

I'll leave you to figure it out. Just leaving my opinions.

[mitschabaude]

@qcomps the suggestion I get from that is that accountUpdate.send() should just add requireSignature(), always (not looking at the permission).
But SmartContract.send() shouldn't because most smart contracts would send with a proof, not a signature.

[mitschabaude]

this requireSignature call which feels out place to, say, a solidity programmer.

Ha! I think that the problem might be that we have the same generic type (AccountUpdate) for both zkApp account updates, and normal user updates. Which are completely different. If we'd create a special type for a "user account update" then we would have methods on it that feel natural, for example because every send from a user needs a signature, so it's added by the method

[mitschabaude]

I am seeing this in the source code:

This is the token send method, not the one we were talking about, where we decided to require the signature by default. (in part because sending tokens from a zkApp needs a more complicated API anyway)