Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add upgradeto action #20

Merged
merged 2 commits into from
Sep 8, 2023
Merged

Add upgradeto action #20

merged 2 commits into from
Sep 8, 2023

Conversation

taokayan
Copy link
Contributor

@taokayan taokayan commented Sep 8, 2023

Resolve #19

Copy link
Contributor

@yarkinwho yarkinwho left a comment

Choose a reason for hiding this comment

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

I am not sure about if we can do it or not but will std::swap work here for substitute in the address (that is, a vector)?

otherwise it looks fine. I will take care of the test after the IT is merged in.

uint64_t id = 0;
impl_contract_table_t contract_table(_self, _self.value);

contract_table.emplace(_self, [&](auto &v) {
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to provide a more meaningful error message if someone calls upgradeto after the table entry with id of 0 was already created (e.g. due to a prior call of upgradeto or upgrade. Right now it goes deep into chainbase code before it would give an confusing error about a uniqueness constraint being violated.

I won't block this PR from being merged in because of this. But maybe @yarkinwho can add that check to this action with a better error message when he adds the tests in his PR (and includes in those tests checks that calling upgrade or upgradeto after they have already been called will fail as expected).

@arhag arhag changed the title Kayan upgradeto Add upgradeto action Sep 8, 2023
@arhag
Copy link
Member

arhag commented Sep 8, 2023

Also, please put a little bit more effort into the PR titles. They will be showing up in the release notes.

@arhag arhag merged commit 8611744 into main Sep 8, 2023
4 checks passed
@arhag arhag deleted the kayan_upgradeto branch September 8, 2023 23:04
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.

Upgradeto action to take the implementation address as parameter
3 participants