Open ascjones opened 10 months ago
My personal take. I think this would be a useful feature to have. I personally do not see any security implications with this feature other than a potential user error, but this can be fixed with extensive documentation.
On the other, hand, introducing this feature will highlight the issue addressed in #1969 (i.e. ink! does not provide immutability in messages intrinsically). Since it would be absolutely valid to have this:
#[ink(message)]
fn update_cell(value: u8) {
Lazy::<u8, ManualKey<127>>::new().set(&value);
}
Therefore, as part of this PR, I believe we should then remove mutability
field from the metadata and advertise ink! code as mutable. Even though this is beyond the scope of this issue, it is affected as well.
My sentiment has always been removing direct storage and code manipulation and provide an encapsulating interface that would handle storage access and code upgrades while inferring the mutability from the message it is called. But again, beyond the scope of this issue.
I think this feature would actually pave the way for the upgradeability interface which I will outline later this week.
On the final note, I think ink::env::set_code_hash::<<Self as ink::env::ContractEnv>::Env>(&code_hash).unwrap()
can be turned into a proc macro set_code_hash!(&code_hash).unwrap()
for shortness.
Therefore, as part of this PR, I believe we should then remove mutability field from the metadata and advertise ink! code as mutable. Even though this is beyond the scope of this issue, it is affected as well.
There still exists the distinction that mutable
messages will automatically persist the top level storage struct upon successful execution. It is a limitation that the mutability does not provide us with any guarantees, if a user chooses to use the underlying APIs, as @xermicus says https://github.com/paritytech/ink/issues/1969#issuecomment-1791374463.
My sentiment has always been removing direct storage and code manipulation and provide an encapsulating interface that would handle storage access and code upgrades while inferring the mutability from the message it is called. But again, beyond the scope of this issue.
At the moment the ink codegen requires the public API for reading/writing from storage. Maybe there is a technique we can use to hide that API from being used directly by the contract author, but I can't think off the top of my head a way to prevent calling the under the covers methods entirely, if they are to be exposed to the codegen.
It is a limitation that the mutability does not provide us with any guarantees, if a user chooses to use the underlying APIs
Indeed, that's why we should discourage the use of the interface in favour of higher-level APIs that should come in future.
At the moment the ink codegen requires the public API for reading/writing from storage. Maybe there is a technique we can use to hide that API from being used directly by the contract author, but I can't think off the top of my head a way to prevent calling the under the covers methods entirely, if they are to be exposed to the codegen.
This is obviously a massive refactoring of the ink_env
crate which is not the main priority atm. Therefore, as mentioned earlier, we should simply provide a better interface that would be favourable to use instead of direct storage operations for upgradeability. I see the design described in this issue as a step forward to it, but only if it will not be used by the developer directly and will be encapsulated as part of some migration function in future.
It's okay to use it now, since it allows devs to "unscrew" the migration as I mentioned in https://github.com/paritytech/ink/pull/1909#issuecomment-1777032680
I've just successfully tested an approach to upgrade contracts with breaking storage changes and it might be used to fix broken contracts using the proposal:
parity_scale_codec::Decode
manually for the struct to decode only the working fieldparity_scale_codec::Encode
manually for the struct to encode only the new fieldThankyou @ShadoySV that is a nice idea for a workaround!
Abstract
Proposing the addition of
ink!
messages which do not enforce reading and decoding the top level[ink(storage)]
struct. This would provide additional flexibility to contract authors, and provides an "escape hatch" in case the data at the root key of the contract cannot be decoded into the top level storage struct.Motivation
ink!
messages are currently required to be methods with a&self
or&mut self
receiver. In both cases the top level contract storage struct is first loaded from storage and passed in as theself
argument. For&mut self
the storage struct is persisted following successful execution of the message logic.One problem with this is that if the storage data at the root storage key of the contract cannot be successfully decoded into the storage struct, then the contract will be permanently* "bricked". This can happen either by the storage being corrupted via a direct write to
set_contract_storage
, or during migration to a new version of a contract viaset_code_hash
where the new version of the contract has an incompatible layout with the data at the root storage key.The following example illustrates:
The call to
migrate_storage
writes directly to the contract storage, upgrading thevalue
from au32
to au64
. A subsequent call toset_code
(or any other message) would now fail because theu64
cannot be decoded into the originalu32
(see https://github.com/paritytech/ink/pull/1897). Note that decoding errors can also occur with other any other invalid/corrupted data.Therefore we need a way to be able to call messages such as
set_code
which do not have the constraint of pre-loading the storage level struct.* The "bricked" contract could be fixed by calling the priveleged
set_code
extrinsic onpallet_contracts
, but this would require the contract author coordinating with thesudo
owner or via governance.Specification
Rust allows "associated functions" in impl blocks which do not take a
self
receiver. These can be used for messages which do not load the root contract storage before message invocation. Using the example above, theset_code
function becomes:This allows the contract code to be updated without first requiring to load the contract state, which would fail.
Considerations
Does this pattern open up any security vulnerabilities, or otherwise introduce a footgun for contract authors? Should we allow contracts to be bricked, and encourage better testing to avoid the eventuality instead?
Alternatively we could encourage writing all contracts using
Lazy
andMapping
values at the top level, which could avoid some of the problems by not expecting any data to be stored at the top level. Of course this would still be affected by storage corruption if any data happens to be written directly to the root storage key.Rationale
The layout of the contract storage can change, either intentionally for migration or accidentally/maliciously by directly writing to contract storage via
ink::env::set_contract_storage
. In this and other cases where we do not want or need to load the root contract state before executing a message,ink!
messages which do not accept aself
instance of the root contract storage should be provided.