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

Burn illegal token and Frozen configuration #250

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

Conversation

welbon
Copy link
Contributor

@welbon welbon commented Mar 18, 2024

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Other information

Copy link
Contributor

@nkysg nkysg Mar 18, 2024

Choose a reason for hiding this comment

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

为啥把代码格式改变,这看不到修改差别了

sources/Account.move Outdated Show resolved Hide resolved
use StarcoinFramework::Config;
use StarcoinFramework::Signer;
Copy link
Contributor

Choose a reason for hiding this comment

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

这感觉没必要修改吧,import换了位置

strategy: u8,
exec_delay: u64) {
let consensus_config = ConsensusConfig::new_consensus_config(uncle_rate_target,
public entry fun propose_update_consensus_config(
Copy link
Contributor

Choose a reason for hiding this comment

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

这些修改添加了啥变量吗?

}

public entry fun do_upgrade_from_v12_to_v13(sender: signer) {
CoreAddresses::assert_genesis_address(&sender);
Copy link
Contributor

Choose a reason for hiding this comment

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

这里也是,是不是就添加了v12_to_v13

const ERR_ADD_CANNOT_BE_CORE_ADDRESS: u64 = 103;
const ERR_REMOVE_ACCOUNT_FAILED: u64 = 102;

public entry fun initialize(sender: signer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

这里iniialize不用entry,一般是放在Genesis.move里面

FrozenConfig::initialize(&sender, frozen_list_v1());
}

public entry fun add_account(sender: signer, account: address) {
Copy link
Contributor

@nkysg nkysg Mar 19, 2024

Choose a reason for hiding this comment

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

这里是说基金会有权利添加非法账号?

Choose a reason for hiding this comment

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

It's kinda dangerous. The entry function can be called directly in transaction.

StdlibUpgradeScripts::do_upgrade_from_v12_to_v13(&genesis_account);

// Initialize Frozen strategy
FrozenConfigStrategy::do_initialize(&association);
Copy link
Contributor

Choose a reason for hiding this comment

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

这个可以直接放到do_upgrade_from_v12_to_v13里面吧

Choose a reason for hiding this comment

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

When doing stdlib upgrading, make sure every vm doing the same work. The balances of accounts when upgrading stdlib could be different according to the upgrading height. In that situation, different node might has different balances, the burning operation might cause inconsistent problem.

public entry fun upgrade_from_v12_to_v13(sender: signer) {
do_upgrade_from_v12_to_v13(&sender);
}
public fun do_upgrade_from_v12_to_v13(sender: &signer) {
Copy link

@simonjiao simonjiao Mar 20, 2024

Choose a reason for hiding this comment

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

We could do a proper initialization when upgrading stdlib, like pushing_new_config then set the address list. Then we could burn the balances in a block triggered by executing a DAO proposal.

);
}

public fun set_account_list(sender: &signer, frozen_account_list: ACL::ACL) {
Copy link

@simonjiao simonjiao Mar 20, 2024

Choose a reason for hiding this comment

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

We can add spec to set_account_list/set_global_frozen to make sure it can be executed properly like the initialize function

}

spec initialize {
aborts_if !Timestamp::is_genesis();

Choose a reason for hiding this comment

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

if we try to initializing frozen config through stdlib upgrading, it should not be a genesis block.

assert!(!ACL::contains(&new_acl, account), Errors::invalid_state(ERR_REMOVE_ACCOUNT_FAILED));
}

public entry fun set_global_frozen(sender: signer, frozen: bool) {

Choose a reason for hiding this comment

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

It could be better to add spec to each function.

Copy link

@simonjiao simonjiao left a comment

Choose a reason for hiding this comment

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

Please checkout the comments, thx.

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