feat(upgradable)!: add `hash` parameter to `up_deploy_code`

[mooori]

Adds the parameter hash: Option<String> to the function up_deploy_code of the Upgradable plugin.

If hash is h, the deployment succeeds only if h equals the hash of the currently staged code. In particular, h must correspond to the base64 encoded sha256 hash of the deployed code. Given the bytes of the deployed code, h can be computed using near-sdk-rs. See for instance the function convert_code_to_deploy_hash used in tests.

Motivation: It helps facilitate some workflows, for example DAO upgrade approval.

BREAKING CHANGE

This is a breaking change since the signature of up_deploy_code is changed.

let promise = up_deploy_code(hash, function_call_args);

[mooori]

If this is a breaking change then I do not see a reason to make the hash optional. I assume the DAO should always sign over a hash

Couldn’t there be other users or use cases which require the Upgradable functionality without hash?

Since up_deploy_code already has an Option<FunctionCallArgs> parameter I thought to add the hash: Option<String> parameter here as well, to keep everything in one function. Though I see the concern regarding the breaking change and I’m also fine with leaving up_deploy_code as is and adding a separate function for the deployment with hash verification.

In this version the escape hatch of the old function is still available, but we could mark it as deprecated and remove it in a future release

My understanding was that the Bridge team requested the hash feature to facilitate their workflows, not due unsafety of the version without hash. If that’s the case, we might not necessarily have to deprecate and remove it.

[birchmd]

not due unsafety of the version without hash. If that’s the case, we might not necessarily have to deprecate and remove it.

When I say "unsafe" I mean with respect to the DAO-controlled workflow. Essentially the DAO is not signing over the actual bytes being deployed (because the transaction is too large), so instead they are signing over the hash of the bytes as part of the transaction that actually triggers the upgrade. If that hash were missing (either because None is an allowed value, or they use the old function without any hash) then they may not be approving the bytes to deploy that they intended to.

In general the flow is not unsafe if the role which triggers the upgrade trusts the role which stages the bytes to deploy. If these roles do not trust each other then the hash is needed to ensure they agree on what bytes are actually being deployed.

Whether or not we keep the old version of the function (without any hash) depends on if we think the case of the roles trusting each other is common enough that including the hash will be unnecessary for many users. Personally I think it is better to always include the hash, even if the roles trust each other, because a redundant check only decreases the probability of making a mistake during the upgrade.

[karim-en]

Replaced Option with hash aeff8256d4e98ef402d3ff6b6dca5785636b86b0