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);

[birchmd]

The code looks good to me, but I have some questions on the design.

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 to make sure the right code is deployed, so having the None escape hatch seems like it creates unnecessary risk.

We could avoid the breaking change by having the hash verification be a separate function in the trait (e.g. up_verified_deploy_code or up_deploy_hash), and again in that function the hash would not be optional. 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 instead of making the breaking change now.

[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
aeff825