Skip to content

Add support for the delayed upgrade#44

Merged
mooori merged 19 commits into
masterfrom
add-upgrade-delay
Feb 3, 2023
Merged

Add support for the delayed upgrade#44
mooori merged 19 commits into
masterfrom
add-upgrade-delay

Conversation

@karim-en
Copy link
Copy Markdown
Collaborator

@karim-en karim-en commented Nov 24, 2022

Implement #20

Added apis:

- up_storage_prefix();
- up_get_delay_status(&self) -> UpgradableDurationStatus;
- up_init_staging_duration(staging_duration: near_sdk::Duration);
- up_stage_update_staging_duration(staging_duration: near_sdk::Duration);
- up_apply_update_staging_duration();

Added storage keys:

- "StagingDuration":  to save the delay duration of deploying staged code.
- "UpdateStagingDuration": to save the staged delay duration update.
- "StagingTimestamp": to save the allowed timestamp to apply the staged duration or code.

@karim-en karim-en added the enhancement New feature or request label Nov 24, 2022
@karim-en karim-en requested a review from sept-en November 24, 2022 19:06
@karim-en karim-en self-assigned this Nov 24, 2022
@karim-en karim-en linked an issue Nov 24, 2022 that may be closed by this pull request
@sept-en sept-en requested review from mfornet and mooori November 24, 2022 20:24
Comment thread near-plugins-derive/src/upgradable.rs Outdated

#[#cratename::only(owner)]
fn up_stage_code(&mut self, #[serializer(borsh)] code: Vec<u8>) {
fn up_stage_code(&mut self, #[serializer(borsh)] code: Vec<u8>, #[serializer(borsh)] delay_timestamp: u64) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Orthogonal to this PR. I wonder if we should avoid borsh here and use json, passing the code in base64. It will be more inefficient, but better for readability.

Copy link
Copy Markdown
Collaborator Author

@karim-en karim-en Nov 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mfornet @sept-en
I've implemented it as base64 here 5c23697, but I worry that the gas limit could be reached faster by deploying a big contract.

The difference is 12TG for the example contract
https://explorer.testnet.near.org/transactions/HhRhMqRxLg2fSSEZ1ND3mBNL7m7wmw4oqqZmzVCPq6M4
https://explorer.testnet.near.org/transactions/7jn1RE5y9VwzvAQ1Q9Mpd5JqnB7KXZfou1NXKe6uWdRd

Comment thread near-plugins-derive/src/upgradable.rs Outdated
@karim-en karim-en marked this pull request as draft November 25, 2022 11:54
@karim-en karim-en marked this pull request as ready for review December 3, 2022 14:14
@karim-en karim-en requested a review from birchmd December 5, 2022 13:10
Copy link
Copy Markdown

@birchmd birchmd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Copy Markdown
Contributor

@mooori mooori left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the README should be updated to explain how to use Upgradable with delay. If I’m not mistaken, it can not be used without delay anymore (up_deploy_code panics if there’s no staging timestamp).

Comment thread near-plugins-derive/src/upgradable.rs Outdated
Comment thread near-plugins-derive/src/upgradable.rs Outdated
Copy link
Copy Markdown
Contributor

@mooori mooori left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about introducing helper functions for storage writes too? It’s a nit, but I think it would improve readability.

#[inline]
fn up_set_duration(key: __UpgradableStorageKey, duration: &near_sdk::Duration) {
  near_sdk::env::storage_write(self.up_storage_key(key).as_ref(), duration.to_be_bytes());
}

// Same for `fn up_set_timestamp`.

These functions would be the counterparts of up_get_{duration, timestamp}.


Please remember to update the Upgradable section in the root README.

Comment thread near-plugins-derive/src/upgradable.rs Outdated
Comment thread near-plugins-derive/src/upgradable.rs Outdated
Comment thread near-plugins-derive/src/upgradable.rs Outdated
Comment thread near-plugins-derive/src/upgradable.rs Outdated
Comment thread near-plugins/src/upgradable.rs Outdated
@karim-en
Copy link
Copy Markdown
Collaborator Author

How about introducing helper functions for storage writes too? It’s a nit, but I think it would improve readability.

#[inline]
fn up_set_duration(key: __UpgradableStorageKey, duration: &near_sdk::Duration) {
  near_sdk::env::storage_write(self.up_storage_key(key).as_ref(), duration.to_be_bytes());
}

// Same for `fn up_set_timestamp`.

These functions would be the counterparts of up_get_{duration, timestamp}.

Please remember to update the Upgradable section in the root README.

fixed in :
6489041
3a1f09a

Comment thread near-plugins-derive/src/upgradable.rs Outdated
Copy link
Copy Markdown
Contributor

@mooori mooori left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using the term delay instead of staging duration in the code? I think that could make things clearer, for instance:

fn up_stage_new_delay(&self, delay: Option<near_sdk::Duration>);

// instead of

fn up_stage_update_staging_duration(&self, staging_duration: Option<near_sdk::Duration>);

There are events for StageCode and DeployCode.

https://github.com/aurora-is-near/near-plugins/blob/6489041834e6f7040f583b6795f15eebfc4cee30/near-plugins/src/upgradable.rs#L90-L95

https://github.com/aurora-is-near/near-plugins/blob/6489041834e6f7040f583b6795f15eebfc4cee30/near-plugins/src/upgradable.rs#L110-L115

I think it would make sense to also have events like StageDelay and SetDelay

Comment thread near-plugins-derive/src/upgradable.rs
Comment thread near-plugins-derive/src/upgradable.rs
Comment thread near-plugins-derive/src/upgradable.rs Outdated
Comment thread near-plugins-derive/src/upgradable.rs Outdated
Comment thread near-plugins-derive/src/upgradable.rs Outdated
@karim-en
Copy link
Copy Markdown
Collaborator Author

karim-en commented Jan 29, 2023

How about using the term delay instead of staging duration in the code? I think that could make things clearer, for instance:

fn up_stage_new_delay(&self, delay: Option<near_sdk::Duration>);

// instead of

fn up_stage_update_staging_duration(&self, staging_duration: Option<near_sdk::Duration>);

There are events for StageCode and DeployCode.

https://github.com/aurora-is-near/near-plugins/blob/6489041834e6f7040f583b6795f15eebfc4cee30/near-plugins/src/upgradable.rs#L90-L95

https://github.com/aurora-is-near/near-plugins/blob/6489041834e6f7040f583b6795f15eebfc4cee30/near-plugins/src/upgradable.rs#L110-L115

I think it would make sense to also have events like StageDelay and SetDelay

The term staging duration is already used in the NEP, NEP implementation, so I am skeptical about adding new terminology

Copy link
Copy Markdown

@birchmd birchmd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should get this merged. @mooori please take another look and let us know if you have any additional comments.

Copy link
Copy Markdown
Contributor

@mooori mooori left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM in general. Left some minor comments that I think should be addressed before merging.

Comment thread near-plugins/src/upgradable.rs Outdated
Comment thread near-plugins/src/upgradable.rs Outdated
Comment thread near-plugins-derive/src/upgradable.rs
Copy link
Copy Markdown
Contributor

@mooori mooori left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karim-en thanks!

@mooori mooori merged commit 244d008 into master Feb 3, 2023
@mooori mooori deleted the add-upgrade-delay branch February 3, 2023 16:58
This was referenced Mar 27, 2026
This was referenced Apr 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Upgradeability: add delay on code upgrade

6 participants