chore: additional changes to propose_update #729

ChaoticTempest · 2024-07-24T21:30:18Z

Follow up to #714

An attached deposit is now required on propose_update where the uploaded config or contract bytes is used to determine how much. This amount currently cannot be withdrawn, so it'll be trapped there until we make more changes to the contract. For us, this just means that there's a cost of about 8N per contract update to mainnet and not more than 0.1N for config updates
Configs are now dynamic so we can make any arbitrary config update without changing the contract code at all
ProposedUpdates now just uses one hashmap to save us the hassle of fetching both the votes and updates at the same time
migrate function will be invoked after a deploy contract
test case where deployed contract has a paniced/failed migrate function which effectively rolls back to the previous state. In this case the update fails and is evicted from being voted upon again

volovyks

LGTM, we will need to better test it. @ppca @ailisp I'm merging this to unblock other PRs, but please, take a look and comment.

volovyks · 2024-07-25T09:39:30Z

chain-signatures/test-contracts/src/lib.rs

@@ -0,0 +1,18 @@
+use near_sdk::{env, near, PanicOnDefault};
+
+// use crate::FailureContractExt;


volovyks · 2024-07-25T09:44:52Z

chain-signatures/contract/tests/tests.rs

    let execution = contract
        .call("propose_update")
        .args_json(serde_json::json!({
-            "contract": vec![1, 2, 3],
+            "code": vec![1, 2, 3],


The test is called test_propose_update_config but we are proposing a new code.

volovyks · 2024-07-25T10:07:07Z

chain-signatures/contract/tests/tests.rs

+
+    dbg!(&execution);
+    let state: mpc_contract::ProtocolContractState = execution.json().unwrap();
+    dbg!(state);


Do we need to check the execution status here?

nope, json().unwrap() does it

volovyks · 2024-07-25T10:11:54Z

chain-signatures/contract/tests/tests.rs

+    // Try calling into state and see if it works after the contract updates with an invalid
+    // contract. It will fail in `migrate` so a state rollback on the contract code should have
+    // happened.
+    let execution = accounts[0]


IMO the end goal in tests for update logic should be:

To make sure that the user-facing API was not changed, make a sign, pk, and other calls. It should prevent us from accidentally breaking the API

To make sure that the upgrade logic is still functioning after the update. It will allow us to fix it with new updates no matter what. The flow can be: deploy the contract, upgrade it, and upgrade it again to see if the upgrade logic is functioning.

volovyks · 2024-07-25T10:12:43Z

chain-signatures/contract/tests/tests.rs

+    // contract. It will fail in `migrate` so a state rollback on the contract code should have
+    // happened.
+    let execution = accounts[0]
+        .call(contract.id(), "state")


This function does not exist on that contract. Is this intentional?

because of the state rollback, the FailureContract does not get deployed, so we end up back at our original contract which has the state function

volovyks · 2024-07-25T10:50:31Z

chain-signatures/contract/src/lib.rs

-        else {
-            return Err(MpcContractError::VoteError(VoteError::UpdateNotFound));
+        let Some(_promise) = self.proposed_updates().do_update(&id, UPDATE_CONFIG_GAS) else {
+            return Err(MpcContractError::from(VoteError::UpdateNotFound));
        };

        Ok(true)


theoretically do_update can fail, but maybe convenient bool result is more important

volovyks · 2024-07-25T11:02:59Z

chain-signatures/contract/src/lib.rs

+    #[private]
+    #[init(ignore_state)]
+    #[handle_result]
+    pub fn migrate() -> Result<Self, MpcContractError> {


We must keep the last deployed version somewhere to ensure migrations are successful.

yeah, I think we should have a list of structs when we're making these breaking changes. We're supposed to be using the versioned contracts for this, but it hasn't been necessary since we haven't released yet

volovyks · 2024-07-25T11:07:35Z

chain-signatures/contract/src/lib.rs

@@ -772,12 +794,27 @@ impl VersionedMpcContract {
        }
    }

+    fn threshold(&self) -> Result<usize, VoteError> {


VoteError looks strange here :/ We may need to reorg the error structure

volovyks · 2024-07-25T11:12:37Z

chain-signatures/contract/src/update.rs

-    votes: HashMap<UpdateId, HashSet<AccountId>>,
-    next_id: u64,
+    entries: HashMap<UpdateId, UpdateEntry>,
+    id: UpdateId,


Isn't next_id a better name?

It's just an id generator. Calling it next_id meant that it could be sequential when it just generates you an arbitrary id

volovyks · 2024-07-25T11:38:05Z

chain-signatures/contract/src/update.rs

-                Update::Contract(code) => promise = promise.deploy_contract(code),
+                Update::Contract(code) => {
+                    // deploy contract then do a `migrate` call to migrate state.
+                    promise = promise.deploy_contract(code).function_call(


Does this work as a batched transaction? Will deployment fail if the function call fails?

yeah, this ends up being a batch call where calling into migrate will roll back the whole entire promise with the deployed contract

github-actions · 2024-07-25T11:40:25Z

Terraform Feature Environment Destroy (dev-729)

Terraform Initialization ⚙️`success`

Terraform Destroy `success`

Show Destroy Plan



No changes. No objects need to be destroyed.

Either you have not created any objects yet or the existing objects were
already deleted outside of Terraform.

Destroy complete! Resources: 0 destroyed.

Pusher: @volovyks, Action: pull_request, Working Directory: ``, Workflow: Terraform Feature Env (Destroy)

ChaoticTempest added 8 commits July 24, 2024 12:23

Made update methods us MpcContractError

07296d0

Made proposed updates more space efficient

84811d3

Added bytes_used for config and code updates

7a3efad

Removed unnecessary near-rng crate

c964817

Added deposit to propose_update

7d1fae2

Make config dynamic for easier changes

88e6ab1

Added proper borsh serialization and payable

30981df

Use to_vec instead for config borsh ser

a772613

ChaoticTempest requested review from volovyks, ailisp and ppca July 24, 2024 21:30

ChaoticTempest added 3 commits July 24, 2024 14:40

Update deposit cost

b9512c1

Clippy

55c5d63

Added propose_update refund diff and migrate

2bd66dd

ChaoticTempest force-pushed the phuong/chore/additional-changes-to-update branch from 82fd209 to 2bd66dd Compare July 24, 2024 22:06

ChaoticTempest added 2 commits July 24, 2024 15:46

Added additional test case for a contract that fails to migrate

ada833c

Update INVALID contract path

52735fc

volovyks approved these changes Jul 25, 2024

View reviewed changes

volovyks merged commit fa6fa9b into develop Jul 25, 2024
4 checks passed

volovyks deleted the phuong/chore/additional-changes-to-update branch July 25, 2024 11:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: additional changes to propose_update #729

chore: additional changes to propose_update #729

ChaoticTempest commented Jul 24, 2024 •

edited

Loading

volovyks left a comment

volovyks Jul 25, 2024

volovyks Jul 25, 2024

volovyks Jul 25, 2024

ChaoticTempest Jul 25, 2024

volovyks Jul 25, 2024

volovyks Jul 25, 2024

ChaoticTempest Jul 25, 2024

volovyks Jul 25, 2024

volovyks Jul 25, 2024

ChaoticTempest Jul 25, 2024 •

edited

Loading

volovyks Jul 25, 2024

volovyks Jul 25, 2024

ChaoticTempest Jul 25, 2024

volovyks Jul 25, 2024

ChaoticTempest Jul 25, 2024

github-actions bot commented Jul 25, 2024

		@@ -0,0 +1,18 @@
		use near_sdk::{env, near, PanicOnDefault};

		// use crate::FailureContractExt;

chore: additional changes to propose_update #729

chore: additional changes to propose_update #729

Conversation

ChaoticTempest commented Jul 24, 2024 • edited Loading

volovyks 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChaoticTempest Jul 25, 2024 • 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

github-actions bot commented Jul 25, 2024

Terraform Feature Environment Destroy (dev-729)

Terraform Initialization ⚙️success

Terraform Destroy success

ChaoticTempest commented Jul 24, 2024 •

edited

Loading

ChaoticTempest Jul 25, 2024 •

edited

Loading

Terraform Initialization ⚙️`success`

Terraform Destroy `success`