Took out a footgun with allowances

[DavidM-D]

Address's #871 .

I expect this is a reasonable final state for the library, but how should I manage the migration?

Create new function names for the new behavior and depreciate the old endpoints?

[austinabell]

Create new function names for the new behavior and depreciate the old endpoints?

That would be the safest way, for sure. A little unfortunate that this small change touches three different endpoints currently. Unclear what the alternative would be, other than just making the change with a major release and documenting the migration/accepting breakages.

[austinabell]

feels like this should be:

Suggested change

	match allowance {
	0 => Allowance::Unlimited,
	x => Allowance::limited(x).expect("This must be non zero"),
	}
	Allowance::limited(allowance).unwrap_or(Allowance::Unlimited)

Sorry if that doesn't compile, I didn't check it, but want to avoid the expect which comes with unnecessary bloat to compiled code size

[DavidM-D]

Thanks, much cleaner too

[ChaoticTempest]

I wonder if we should add a note that a zero allowance is allowed outside of promises. A zero allowance is allowed with add keys calls through RPC into the chain. This is just a special case of the host functions defining allowance as a ptr to a u64

[austinabell]

wait, what? You're saying you can create a function call access key without an allowance through rpc?

[ChaoticTempest]

yup. For example the following workspaces code:

use serde_json::json;
use workspaces::types::{KeyType, SecretKey};
use workspaces::{AccessKey, Account};

const STATUS_MSG_WASM_FILEPATH: &str = "./examples/res/status_message.wasm";

#[tokio::main]
async fn main() -> anyhow::Result<()> {
    let worker = workspaces::sandbox().await?;
    let wasm = std::fs::read(STATUS_MSG_WASM_FILEPATH)?;
    let contract = worker.dev_deploy(&wasm).await?;

    let sk = SecretKey::from_seed(KeyType::ED25519, "test");
    let pk = sk.public_key();
    let ak = AccessKey::function_call_access(contract.id(), &["set_status"], Some(0));
    let account = Account::from_secret_key(contract.id().clone(), sk, &worker);

    contract
        .batch()
        .add_key(pk, ak)
        .transact()
        .await?
        .into_result()?;

    let outcome = account
        .call(contract.id(), "set_status")
        .args_json(json!({
            "message": "hello_world",
        }))
        .transact()
        .await?;
    println!("set_status: {:?}", outcome);

    Ok(())
}

would yield the error when calling into set_status:

Error: unable to broadcast the transaction to the network

Caused by:
    handler error: [An error happened during transaction execution: InvalidAccessKeyError(NotEnoughAllowance { account_id: AccountId("dev-20221130043518-92793473638831"), public_key: ed25519:DcA2MzgpJbrUATQLLceocVckhhAqrkingax4oJ9kZ847, allowance: 0, cost: 1561286367017332100000 })]

[austinabell]

honestly had a vague intuition that this path would use the same logic as the runtime host function. Yeah, could document the difference. Honestly, there could be a reason (albeit bad) to add a key with no allowance just to record it as on-chain data. It's how some protocols verify accounts given there is no arbitrary signing right now

[austinabell]

naming might be a bit confusing but I don't know of a better solution here