[pallet-revive] Add Pallet::get_storage_keys#9625
Conversation
AndreiEres
added
the
T7-smart_contracts
|
/cmd prdoc --audience runtime_user --bump minor |
AndreiEres
requested review from
athei,
filip-parity,
pgherveou,
pkhry and
smiasojed
…time_user --bump minor'
substrate/frame/revive/src/lib.rs
Outdated
| /// | ||
| /// Returns `Ok(Vec<Vec<u8>>)` if the contract exists, containing all storage keys | ||
| /// associated with the contract. Otherwise returns `Err(ContractAccessError)` | ||
| fn get_storage_keys(address: H160) -> Result<Vec<Vec<u8>>, ContractAccessError>; |
There was a problem hiding this comment.
Feels weird that both this function and the one from here https://github.com/paritytech/polkadot-sdk/pull/9617/files, needs to be added to the public APIs just for forge test,
Would it make more sense to actually have something like ReviveAPIExtension, ReviveAPITestExtension trait and have all of them there ?
There was a problem hiding this comment.
I think that's a good idea
Seems like the guys don't like it
#9617 (comment)
There was a problem hiding this comment.
Nit I would just use an Option here
Maybe we should use a BoundedVec just to be safe, so that this can not be exploited 🤔
| let _ = <Test as Config>::Currency::set_balance(&ALICE, 1_000_000); | ||
|
|
||
| let keys = Pallet::<Test>::get_storage_keys(ALICE_ADDR); | ||
| assert_err!(keys, crate::ContractAccessError::DoesntExist); | ||
|
|
||
| let Contract { addr, .. } = | ||
| builder::bare_instantiate(Code::Upload(code)).build_and_unwrap_contract(); | ||
|
|
||
| let keys = Pallet::<Test>::get_storage_keys(addr).unwrap(); | ||
| assert!(keys.is_empty()); | ||
|
|
||
| let contract = AccountInfo::<Test>::load_contract(&addr).unwrap(); | ||
| let child_info = contract.child_trie_info(); | ||
| let storage_key = child_info.storage_key(); | ||
| let key1 = b"key_1"; | ||
| let key2 = b"key_2"; | ||
| sp_io::default_child_storage::set(storage_key, key1, &[]); | ||
| sp_io::default_child_storage::set(storage_key, key2, &[]); | ||
|
|
||
| let keys = Pallet::<Test>::get_storage_keys(addr).unwrap(); | ||
| assert_eq!(keys.len(), 2); | ||
| assert!(keys.contains(&key1.to_vec())); | ||
| assert!(keys.contains(&key2.to_vec())); |
There was a problem hiding this comment.
this is not the correct way to write the test,
- create a fixture contract similar to fixtures/contracts/store_call.rs where you set a bunch of keys (maybe in the deploy function)
- then check for these keys
note thatyou will get the hashed keys in your Vec
pgherveou
left a comment
pgherveou
left a comment
There was a problem hiding this comment.
This works but this will give you the hashed key
| let child_info = contract_info.child_trie_info(); | ||
| let mut keys = Vec::new(); | ||
| let mut previous_key = Vec::new(); | ||
|
|
||
| loop { | ||
| let next_key = | ||
| sp_io::default_child_storage::next_key(child_info.storage_key(), &previous_key); | ||
|
|
||
| let Some(key) = next_key else { | ||
| break; | ||
| }; | ||
| keys.push(key.clone()); | ||
| previous_key = key; | ||
| } | ||
|
|
||
| Some(keys) |
There was a problem hiding this comment.
a bit of a shame that there is not an Iterator for this
| $crate::Pallet::<Self>::get_storage(address, key) | ||
| } | ||
|
|
||
| fn get_storage_keys(address: $crate::H160) -> Option<Vec<Vec<u8>>> { |
There was a problem hiding this comment.
maybe call it get_storage_hashed_keys, since that's what you are getting
|
Not needed anymore see rationale in paritytech/foundry-polkadot#274. |
4 participants
Part of #9553
See paritytech/foundry-polkadot#274
Returns all the storage keys associated with the contract.
Integration
Should not affect downstream projects.