feat: add trie key global contract code #12766
Conversation
pugachAG
force-pushed
the
global-contracts-trie-storage
branch
from
63ce420 to
d7a65e6
Compare
pugachAG
force-pushed
the
global-contracts-trie-storage
branch
from
d7a65e6 to
e1d15ac
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12766 +/- ##
=======================================
Coverage 70.51% 70.52%
=======================================
Files 846 846
Lines 174633 174665 +32
Branches 174633 174665 +32
=======================================
+ Hits 123149 123175 +26
Misses 46262 46262
- Partials 5222 5228 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| // Global contract code is not a part of account, so ignoring it as well. | ||
| TrieKey::GlobalContractCode { .. } => {} |
There was a problem hiding this comment.
I was thinking of a global contract as the same thing as a normal contract but deployed on all shards so I imagined the handling would be similar for TrieKey::ContractCode and TrieKey::GlobalContractCode
What exactly do you mean by "not a part of account"?
There was a problem hiding this comment.
StateChanges represents to updates to some parts of user account, global contract is more of a part of a global shard state, so it handled differently
| @@ -193,6 +230,9 @@ pub enum TrieKey { | |||
| receiving_shard: ShardId, | |||
| index: u64, | |||
| }, | |||
| GlobalContractCode { | |||
| identifier: GlobalContractCodeIdentifier, | |||
There was a problem hiding this comment.
Would be great to add a few tests for this new key and its implementation
There was a problem hiding this comment.
Can you add a test for the resharding logic? For reference please see the resharding_v3 testloop. I think it should be sufficient to deploy a global contract before resharing and call it in both children after resharding.
cc @staffik This may also trigger the negative refcount issue / wrong refcount accounting during resharding.
| @@ -62,6 +62,7 @@ pub mod col { | |||
| pub const BUFFERED_RECEIPT_GROUPS_QUEUE_DATA: u8 = 16; | |||
| /// A single item of `ReceiptGroupsQueue`. Values are of type `ReceiptGroup`. | |||
| pub const BUFFERED_RECEIPT_GROUPS_QUEUE_ITEM: u8 = 17; | |||
| pub const GLOBAL_CONTRACT_CODE: u8 = 18; | |||
There was a problem hiding this comment.
nit: Can you add a comment including the value type (like above), please?
| } | ||
| } | ||
|
|
||
| pub fn append_into(&self, buf: &mut Vec<u8>) { |
There was a problem hiding this comment.
Why not use borsh::to_vec().unwrap() here instead of defining functions like append_into, discriminator, etc.?
There was a problem hiding this comment.
TrieKey::AccessKey has a similar pattern
| @@ -53,7 +53,8 @@ fn boundary_account_to_intervals( | |||
| col::DELAYED_RECEIPT_OR_INDICES | |||
| | col::PROMISE_YIELD_INDICES | |||
| | col::PROMISE_YIELD_TIMEOUT | |||
| | col::BANDWIDTH_SCHEDULER_STATE => { | |||
| | col::BANDWIDTH_SCHEDULER_STATE | |||
| | col::GLOBAL_CONTRACT_CODE => { | |||
There was a problem hiding this comment.
I wonder if you hit the // Poor man's exhaustive check for handling all column types. 😛
Might not be super necessary as we have a test in core/store/src/trie/ops/resharding.rs to check if the key, value are copied to both children. |
Part of #12715.
For each global contract we store its code replicated on every shard. This PR adds a trie key to support that.