feat: add receipt action for deploying global contract code #12737

pugachAG · 2025-01-14T20:18:54Z

This PR includes mostly wiring, actual implementation will be added separately to make it easier to review.
I explicitly don't want to introduce compile time feature for this as it makes code much harder to work with. validate_action makes sure that newly added action cannot be used before stabilisation.

Part of #12715.

codecov · 2025-01-14T20:38:23Z

Codecov Report

Attention: Patch coverage is 1.31579% with 75 lines in your changes missing coverage. Please review.

Project coverage is 70.70%. Comparing base (8806e63) to head (2dde6f9).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
runtime/runtime/src/verifier.rs	0.00%	22 Missing ⚠️
core/primitives/src/action/mod.rs	0.00%	16 Missing ⚠️
core/primitives/src/views.rs	0.00%	15 Missing ⚠️
runtime/runtime/src/actions.rs	0.00%	9 Missing ⚠️
runtime/runtime/src/config.rs	0.00%	6 Missing ⚠️
chain/rosetta-rpc/src/adapters/mod.rs	0.00%	3 Missing ⚠️
runtime/runtime/src/lib.rs	0.00%	2 Missing ⚠️
core/primitives-core/src/version.rs	0.00%	1 Missing ⚠️
tools/state-viewer/src/contract_accounts.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #12737      +/-   ##
==========================================
- Coverage   70.74%   70.70%   -0.04%     
==========================================
  Files         848      848              
  Lines      174301   174376      +75     
  Branches   174301   174376      +75     
==========================================
- Hits       123303   123288      -15     
- Misses      45856    45943      +87     
- Partials     5142     5145       +3

Flag	Coverage Δ
backward-compatibility	`0.16% <0.00%> (-0.01%)`	⬇️
db-migration	`0.16% <0.00%> (-0.01%)`	⬇️
genesis-check	`1.35% <0.00%> (-0.01%)`	⬇️
linux	`69.19% <1.31%> (-0.05%)`	⬇️
linux-nightly	`70.26% <1.31%> (-0.05%)`	⬇️
pytests	`1.65% <0.00%> (-0.01%)`	⬇️
sanity-checks	`1.46% <0.00%> (-0.01%)`	⬇️
unittests	`70.53% <1.31%> (-0.04%)`	⬇️
upgradability	`0.20% <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.

shreyan-gupta

Looks good!

shreyan-gupta · 2025-01-15T11:05:06Z

runtime/runtime/src/verifier.rs

+            version: current_protocol_version,
+        });
+    }
+    if action.code.len() as u64 > limit_config.max_contract_size {


Confirming, is this the only check we need in the verifier? I'm guessing it's based off the above validate_deploy_contract_action function?

yes, I've copied this from verifier for the regular deploy contract action, I believe we should enforce the same limits for the global contract code

tools/protocol-schema-check/res/protocol_schema.toml

shreyan-gupta · 2025-01-15T11:16:29Z

core/primitives/src/action/mod.rs

@@ -216,6 +247,7 @@ pub enum Action {
    DeleteKey(Box<DeleteKeyAction>),
    DeleteAccount(DeleteAccountAction),
    Delegate(Box<delegate::SignedDelegateAction>),
+    DeployGlobalContract(DeployGlobalContractAction),
    #[cfg(feature = "protocol_feature_nonrefundable_transfer_nep491")]


Sounds like protocol_feature_nonrefundable_transfer_nep491 isn't stable yet and only potentially as part of nightly. Just wanted to make sure the ordering is not going to mess up BorshSerialize

correct, since nonrefundable_transfer is not stable yet we want to add our feature before that, otherwise stabilising nonrefundable_transfer later will break borsch serialization

wacban

LGTM assuming the design part is agreed upon.

wacban · 2025-01-15T11:11:21Z

core/primitives-core/src/version.rs

@@ -286,6 +287,7 @@ impl ProtocolFeature {
            ProtocolFeature::ExcludeExistingCodeFromWitnessForCodeLen => 148,
            ProtocolFeature::BlockHeightForReceiptId => 149,
            // Place features that are not yet in Nightly below this line.
+            ProtocolFeature::GlobalContracts => 200,


Why not add it to nightly?

I wanted to move it to the nightly separately when I have the complete implementation, to avoid dealing with potential test failures because of unimplemented todos

wacban · 2025-01-15T11:12:49Z

core/primitives/src/views.rs

+                let code = hash(&action.code).as_ref().to_vec();
+                ActionView::DeployGlobalContract { code, link_account: action.link_account }


nit: Can you rename code in DeployGlobalContract to code_hash please? Or even better rename it and change the type to CryptoHash.

this is the weird part I've copied from DeployContract action that I don't really understand: when converting action to the view we hash the contract, but when converting view to action we do not. I assume it works somehow, so I preserved the logic.

wacban · 2025-01-15T11:16:56Z

core/primitives/src/action/mod.rs

+pub struct DeployGlobalContractAction {
+    /// WebAssembly binary
+    #[serde_as(as = "Base64")]
+    pub code: Vec<u8>,
+
+    /// Enables global contract code to referenced by owner account id
+    /// instead of code hash
+    pub link_account: bool,
+}


This seems like a design choice, I'm not familiar enough to comment on this.

let me know if it is unclear, maybe I'd better rename this or add more comment

nagisa · 2025-01-15T11:21:38Z

core/primitives/src/action/mod.rs

+
+    /// Enables global contract code to referenced by owner account id
+    /// instead of code hash
+    pub link_account: bool,


Rather than a bool, consider a field of bitflags. Those are more extensible without affecting the protocol as much in case we want to implement additional behaviours for the action.

nagisa · 2025-01-15T11:34:59Z

runtime/runtime/src/pipelining.rs

@@ -189,7 +189,8 @@ impl ReceiptPreparationPipeline {
                | Action::Stake(_)
                | Action::AddKey(_)
                | Action::DeleteKey(_)
-                | Action::DeleteAccount(_) => {}
+                | Action::DeleteAccount(_)
+                | Action::DeployGlobalContract(_) => {}


This needs to ensure that any of the FunctionCall actions involving the contract deployed by DeployGlobalContract in the current chunk are not pipelined (see the blocking logic for regular deploys.) Probably alright to punt on the implementation here to later, but putting it alongside the "no handling for these" chunk of actions runs a significant risk of forgetting about this.

Global contract deploys only take effect in the next block, so those won't affect pipelining. The actual deploy (saving to the state) will be implemented with a separate receipt type later, and I will add proper handling for that in the pipelining code.

pugachAG · 2025-01-15T13:53:00Z

I've just pushed 650d9aa with the following changes:

use enum instead of bool flag
split the view into 2 separate actions for deploying by code hash and by account id to keep user-facing API more neat

This PR is similar to #12737, but for using global contracts. Part of #12716.

pugachAG requested review from shreyan-gupta and stedfn January 14, 2025 20:18

pugachAG requested a review from a team as a code owner January 14, 2025 20:18

pugachAG force-pushed the add-deploy-global-contract-action branch from d807d7b to 9edff75 Compare January 14, 2025 20:21

impl

a2e35d0

pugachAG force-pushed the add-deploy-global-contract-action branch from 9edff75 to a2e35d0 Compare January 14, 2025 20:49

pugachAG requested a review from nagisa January 15, 2025 11:16

shreyan-gupta approved these changes Jan 15, 2025

View reviewed changes

wacban approved these changes Jan 15, 2025

View reviewed changes

nagisa reviewed Jan 15, 2025

View reviewed changes

improve API

650d9aa

fix schema

cf0fe29

pugachAG added this pull request to the merge queue Jan 15, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 15, 2025

pugachAG added this pull request to the merge queue Jan 15, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 15, 2025

pugachAG added this pull request to the merge queue Jan 15, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 15, 2025

Merge branch 'master' into add-deploy-global-contract-action

2dde6f9

pugachAG enabled auto-merge January 15, 2025 20:01

pugachAG added this pull request to the merge queue Jan 15, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 15, 2025

pugachAG added this pull request to the merge queue Jan 16, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 16, 2025

wacban added this pull request to the merge queue Jan 16, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 16, 2025

pugachAG added this pull request to the merge queue Jan 16, 2025

Merged via the queue into master with commit 0a57709 Jan 16, 2025
26 of 28 checks passed

pugachAG deleted the add-deploy-global-contract-action branch January 16, 2025 15:13

pugachAG mentioned this pull request Jan 16, 2025

feat: add receipt action for using global contract code #12749

Merged

github-merge-queue bot pushed a commit that referenced this pull request Jan 16, 2025

feat: add receipt action for using global contract code (#12749)

8067cb6

This PR is similar to #12737, but for using global contracts. Part of #12716.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add receipt action for deploying global contract code #12737

feat: add receipt action for deploying global contract code #12737

pugachAG commented Jan 14, 2025

codecov bot commented Jan 14, 2025 •

edited

Loading

shreyan-gupta left a comment

shreyan-gupta Jan 15, 2025

pugachAG Jan 15, 2025

shreyan-gupta Jan 15, 2025

pugachAG Jan 15, 2025

wacban left a comment

wacban Jan 15, 2025

pugachAG Jan 15, 2025

wacban Jan 15, 2025

pugachAG Jan 15, 2025

wacban Jan 15, 2025

pugachAG Jan 15, 2025 •

edited

Loading

nagisa Jan 15, 2025

nagisa Jan 15, 2025

pugachAG Jan 15, 2025

pugachAG commented Jan 15, 2025 •

edited

Loading

		let code = hash(&action.code).as_ref().to_vec();
		ActionView::DeployGlobalContract { code, link_account: action.link_account }

feat: add receipt action for deploying global contract code #12737

feat: add receipt action for deploying global contract code #12737

Conversation

pugachAG commented Jan 14, 2025

codecov bot commented Jan 14, 2025 • edited Loading

Codecov Report

shreyan-gupta 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

wacban 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

pugachAG Jan 15, 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

pugachAG commented Jan 15, 2025 • edited Loading

codecov bot commented Jan 14, 2025 •

edited

Loading

pugachAG Jan 15, 2025 •

edited

Loading

pugachAG commented Jan 15, 2025 •

edited

Loading