feat: global contract support for accounts #12877

stedfn · 2025-02-05T16:24:24Z

This PR introduces a new AccountContract field in the AccountV2 struct, replacing code_hash and being defined as follows:

pub enum AccountContract {
    None,
    Local(CryptoHash),
    Global(CryptoHash),
    GlobalByAccount(AccountId),
}

It is being used to differentiate between the types of contracts stored for the Account, if any.

To maintain backward compatibility, two new optional fields for global contracts are also added in the SerdeAccount struct.

Part of #12716

codecov · 2025-02-05T16:57:43Z

Codecov Report

Attention: Patch coverage is 88.47584% with 31 lines in your changes missing coverage. Please review.

Project coverage is 70.44%. Comparing base (20ea311) to head (1ae6226).
Report is 7 commits behind head on master.

Files with missing lines	Patch %	Lines
core/primitives-core/src/account.rs	89.37%	14 Missing and 3 partials ⚠️
core/primitives/src/views.rs	69.23%	4 Missing ⚠️
integration-tests/src/node/mod.rs	0.00%	3 Missing ⚠️
runtime/runtime/src/state_viewer/mod.rs	85.71%	0 Missing and 2 partials ⚠️
chain/rosetta-rpc/src/lib.rs	0.00%	1 Missing ⚠️
runtime/runtime/src/lib.rs	80.00%	1 Missing ⚠️
runtime/runtime/src/verifier.rs	0.00%	1 Missing ⚠️
tools/amend-genesis/src/lib.rs	83.33%	0 Missing and 1 partial ⚠️
tools/fork-network/src/cli.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #12877      +/-   ##
==========================================
+ Coverage   70.41%   70.44%   +0.03%     
==========================================
  Files         854      854              
  Lines      174571   174765     +194     
  Branches   174571   174765     +194     
==========================================
+ Hits       122927   123117     +190     
- Misses      46408    46409       +1     
- Partials     5236     5239       +3

Flag	Coverage Δ
backward-compatibility	`0.35% <0.00%> (-0.01%)`	⬇️
db-migration	`0.35% <0.00%> (-0.01%)`	⬇️
genesis-check	`1.42% <19.60%> (+0.01%)`	⬆️
linux	`70.04% <88.47%> (+0.04%)`	⬆️
linux-nightly	`70.08% <88.47%> (+0.03%)`	⬆️
pytests	`1.73% <19.60%> (+0.01%)`	⬆️
sanity-checks	`1.54% <19.60%> (+0.01%)`	⬆️
unittests	`70.27% <88.47%> (+0.03%)`	⬆️
upgradability	`0.35% <0.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

core/primitives-core/src/account.rs

pugachAG · 2025-02-06T08:46:43Z

core/primitives-core/src/account.rs

+    None,
+    LocalContract(CryptoHash),
+    GlobalContract(CryptoHash),
+    GlobalContractByAccount(AccountId, CryptoHash),


what does CryptoHash correspond to here?

The hash of the contract deployed from the AccountId. I was thinking we need to know if the account has an older contract version deployed from the AccountId

We do not want to store that information as part of user's account. That is not needed, but also we would have to update all accounts when the contract is updated which is not feasible.

we would have to update all accounts when the contract is updated which is not feasible

actually aren't automated upgrades a security concern? shouldn't users manually upgrade after they check the new code?

nope, user accepts such risk when using contract deployed by AccountId, otherwise they should use global contract hash

core/primitives-core/src/account.rs

pugachAG · 2025-02-06T13:08:45Z

core/primitives-core/src/account.rs

-            Self::V1(account) => account.code_hash = code_hash,
-            Self::V2(account) => account.code_hash = code_hash,
+            Self::V1(_) => {
+                let account_v2 = AccountV2 {


AccountV2 has serialization overhead comparing to V1, so we should avoid conversion when possible. So please add additional check for contract being None or Local and do not convert to v2 in such cases. Also would be nice to add tests for that.

should it also get downgraded if possible?

good question! let's not do that for now to keep it simple since I don't expect that to happen frequently

pugachAG · 2025-02-06T13:19:04Z

core/primitives-core/src/account.rs

+}
+
+impl AccountContract {
+    pub fn to_code_hash(&self) -> CryptoHash {


replacing all previous usages of Account::code_hash with to_code_hash is very dangerous since existing clients only expect local code to be there. For example runtime might issues storage reads for TrieKey::ContractCode which would not be correct for global contract. Please replace this with fn local_code(&self) -> Option<CryptoHash> and update clients accordingly.

core/primitives/src/views.rs

runtime/runtime/src/verifier.rs

core/primitives-core/src/account.rs

stedfn · 2025-02-06T15:52:57Z

runtime/runtime/src/pipelining.rs

@@ -136,7 +136,7 @@ impl ReceiptPreparationPipeline {
                }
                Action::FunctionCall(function_call) => {
                    let Some(account) = &**account else { continue };
-                    let code_hash = account.code_hash();
+                    let Some(code_hash) = account.contract().local_code() else { continue }; // TODO: deal with global contract call


@pugachAG I am supposed to deal with global contract function calls here right?

add todo! referencing #12884

I made it a comment with a continue because a test was failing

pugachAG · 2025-02-06T15:56:27Z

please add a detailed description for this PR

core/primitives-core/src/account.rs

core/primitives/src/views.rs

pugachAG

overall LGTM, a couple more nits to address

pugachAG · 2025-02-07T09:50:56Z

chain/jsonrpc/jsonrpc-tests/Cargo.toml

@@ -45,6 +46,7 @@ nightly = [
  "near-jsonrpc/nightly",
  "near-network/nightly",
  "near-o11y/nightly",
+  "near-primitives-core/nightly",


why do we need dependency updates?

this script was complaining

core/primitives-core/src/account.rs

pugachAG · 2025-02-07T10:04:13Z

core/store/src/db/colddb.rs

@@ -208,7 +208,7 @@ mod test {
            }
        }
        fn hash(chunk: &[u8]) -> String {
-            crate::CryptoHash::from(chunk.try_into().unwrap()).to_string()
+            crate::CryptoHash::try_from(chunk).unwrap().to_string()


does this change belong to this pr? 🤔

Oh not anymore, will revert. It was needed before when I added a new From trait implementation for CryptoHash because the compiler did not know which from implementation to use

test-utils/testlib/src/runtime_utils.rs

…ear/nearcore into stefan/global_contract_account_support

pugachAG · 2025-02-07T12:55:15Z

core/primitives/src/views.rs

+        let (global_contract_hash, global_contract_account_id) = match account.contract() {
+            AccountContract::Global(contract) => (Some(contract), None),
+            AccountContract::GlobalByAccount(account_id) => (None, Some(account_id)),
+            _ => (None, None),


please explicitly list all other variants, using _ => is a dangerous patter since adding new enum variant which might be relevant here can be missed

pugachAG · 2025-02-07T12:58:02Z

core/primitives-core/src/account.rs

+
+    #[inline]
+    pub fn local_contract_hash(&self) -> Option<CryptoHash> {
+        match self.contract() {


nit: in the current implementation calling self.contract() results in potentially copying AccountId, would be nice to address that somehow

stedfn added 3 commits February 5, 2025 17:23

global contract support

d81c640

remove duplicate

05887a2

clippy

f2fd27b

stedfn requested a review from pugachAG February 5, 2025 16:42

stedfn marked this pull request as ready for review February 5, 2025 16:42

stedfn requested a review from a team as a code owner February 5, 2025 16:42

pugachAG requested changes Feb 5, 2025

View reviewed changes

core/primitives-core/src/account.rs Outdated Show resolved Hide resolved

stedfn added 5 commits February 5, 2025 22:51

.

498a9ad

simplify

a95fe45

.

625d592

fix

e4b30e6

schema

b2d8efa

pugachAG requested changes Feb 6, 2025

View reviewed changes

stedfn and others added 3 commits February 6, 2025 12:18

address comments

e73e9d9

schema

506d964

Merge branch 'master' into stefan/global_contract_account_support

f995762

pugachAG requested changes Feb 6, 2025

View reviewed changes

stedfn added 5 commits February 6, 2025 14:30

fix

a285285

comments

28c6ea1

fmt

115b53d

style

6baaef7

.

0cc098a

stedfn commented Feb 6, 2025

View reviewed changes

reference todo

4d9bbc9

pugachAG requested changes Feb 6, 2025

View reviewed changes

core/primitives-core/src/account.rs Outdated Show resolved Hide resolved

core/primitives/src/views.rs Outdated Show resolved Hide resolved

stedfn added 2 commits February 6, 2025 17:48

.

e73e627

comments

897985e

pugachAG requested changes Feb 7, 2025

View reviewed changes

stedfn and others added 5 commits February 7, 2025 11:47

comments

102e4a9

introduce local_contract_hash

b4f5ed9

Merge branch 'master' into stefan/global_contract_account_support

f24d365

rename tests

ad1f781

Merge branch 'stefan/global_contract_account_support' of github.com:n…

1ca7b96

…ear/nearcore into stefan/global_contract_account_support

pugachAG approved these changes Feb 7, 2025

View reviewed changes

stedfn added 3 commits February 7, 2025 14:05

schema

99cfe13

address comments

09da9db

fix

1ae6226

stedfn added this pull request to the merge queue Feb 10, 2025

Merged via the queue into master with commit e01e2cf Feb 10, 2025
27 of 29 checks passed

stedfn deleted the stefan/global_contract_account_support branch February 10, 2025 09:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: global contract support for accounts #12877

feat: global contract support for accounts #12877

stedfn commented Feb 5, 2025 •

edited

Loading

codecov bot commented Feb 5, 2025 •

edited

Loading

pugachAG Feb 6, 2025

stedfn Feb 6, 2025 •

edited

Loading

pugachAG Feb 6, 2025

stedfn Feb 6, 2025

pugachAG Feb 6, 2025

pugachAG Feb 6, 2025

stedfn Feb 6, 2025

pugachAG Feb 6, 2025

pugachAG Feb 6, 2025

stedfn Feb 6, 2025

pugachAG Feb 6, 2025

stedfn Feb 6, 2025

pugachAG commented Feb 6, 2025

pugachAG left a comment

pugachAG Feb 7, 2025

stedfn Feb 7, 2025

pugachAG Feb 7, 2025

stedfn Feb 7, 2025

pugachAG Feb 7, 2025

pugachAG Feb 7, 2025

feat: global contract support for accounts #12877

feat: global contract support for accounts #12877

Conversation

stedfn commented Feb 5, 2025 • edited Loading

codecov bot commented Feb 5, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

stedfn Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pugachAG commented Feb 6, 2025

pugachAG left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stedfn commented Feb 5, 2025 •

edited

Loading

codecov bot commented Feb 5, 2025 •

edited

Loading

stedfn Feb 6, 2025 •

edited

Loading