Skip to content

docs(Upgradable): code removal after deplyoment#88

Merged
mooori merged 3 commits into
masterfrom
docs-up-rm-code
Mar 9, 2023
Merged

docs(Upgradable): code removal after deplyoment#88
mooori merged 3 commits into
masterfrom
docs-up-rm-code

Conversation

@mooori
Copy link
Copy Markdown
Contributor

@mooori mooori commented Mar 8, 2023

Makes it more explicit how staged code can be removed after it was deployed. Mentions the benefit of freeing storage.

Related to this issue and its comment.

@mooori mooori marked this pull request as ready for review March 8, 2023 16:46
@mooori mooori requested a review from birchmd March 8, 2023 16:46
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.

The change itself looks good to me. Some other random thoughts about this:

  • How much gas does it actually cost to remove an entry from storage? If it is not much then maybe this argument about saving gas is not as relevant.
  • Should we recommend if users want to atomically update and remove then they could submit a batch transaction that does both?
  • Should we recommend that developers ensure upgrades are always idempotent since it is theoretically possible an upgrade could be applied multiple times if it is not removed immediately?

@mooori
Copy link
Copy Markdown
Contributor Author

mooori commented Mar 9, 2023

How much gas does it actually cost to remove an entry from storage? If it is not much then maybe this argument about saving gas is not as relevant.

The cost of storage_remove grows with the size of the value that is removed due to StorageRemoveRetValueByte. Its cost is somewhere between the cost of StorageReadValueByte and StoragWriteValueByte. I assume that justifies the gas saving arguments and not deleting the code automatically.

Should we recommend that developers ensure upgrades are always idempotent

I guess there could be use cases that need a migration function which is not idempotent. For instance if new state is required to depend on the time of migration or on some external value that changes in time. Mentioning the risks of stale code and repeatedly applied upgrades are sufficient warnings, I think.

Should we recommend if users want to atomically update and remove then they could submit a batch transaction that does both?

Since up_deploy_code returns a promise, combining it with other function calls in a batch transaction can have interesting outcomes. I’ve added a test to demonstrate that (3094276) and updated docs (774147b).

I think the scope of this PR can remain docs, since the new test is kind of a doc test or demo. Though it has to be in near-plugins-derive/tests since it requires the test infrastructure.

@mooori mooori requested a review from birchmd March 9, 2023 11:01
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.

Looks good to me. Thanks for the clarifications @mooori !

@mooori mooori merged commit b76e12f into master Mar 9, 2023
@mooori mooori deleted the docs-up-rm-code branch March 9, 2023 14:50
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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants