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.

Sign up for GitHub

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

Jump to bottom

feat: Secondary miner issurance, split DAO as a separate contract #1094

Merged
merged 13 commits into from
Jun 28, 2019
Merged

feat: Secondary miner issurance, split DAO as a separate contract #1094

merged 13 commits into from
Jun 28, 2019

Conversation

xxuejie
Copy link
Collaborator

@xxuejie xxuejie commented Jun 22, 2019 •
edited
Loading

TODO:

  • tests

This PR is still in WIP progress, it's lacking fixes for the tests so CI is doomed to fail. however it serves 2 purposes:

Now all the tests have been fixed.

@xxuejie xxuejie requested a review from a team June 22, 2019 01:27
@nervos-bot
Copy link

nervos-bot bot commented Jun 22, 2019

@doitian is assigned as the chief reviewer

@xxuejie xxuejie force-pushed the xxuejie/nervosdao-refactor branch 4 times, most recently from 29bcc70 to f1f838a Compare June 22, 2019 02:36
doitian
doitian reviewed Jun 24, 2019
View reviewed changes
db/src/rocksdb.rs Outdated Show resolved Hide resolved
doitian
doitian reviewed Jun 25, 2019
View reviewed changes
util/dao/utils/src/lib.rs Outdated Show resolved Hide resolved
doitian
doitian reviewed Jun 25, 2019
View reviewed changes
resource/specs/dev.toml Outdated Show resolved Hide resolved
@doitian doitian added b:consensus Break consensus b:database Break database schema breaking change The feature breaks consensus, database, message schema or RPC interface. labels Jun 25, 2019
@doitian

This comment has been minimized.

Sign in to view
@xxuejie xxuejie force-pushed the xxuejie/nervosdao-refactor branch from f1f838a to 95327c7 Compare June 25, 2019 08:42
doitian
doitian reviewed Jun 25, 2019
View reviewed changes
util/dao/src/lib.rs Outdated Show resolved Hide resolved
util/dao/src/lib.rs Show resolved Hide resolved
util/dao/src/lib.rs Outdated Show resolved Hide resolved
util/dao/src/lib.rs Show resolved Hide resolved
@xxuejie xxuejie force-pushed the xxuejie/nervosdao-refactor branch from 95327c7 to 6c9262e Compare June 26, 2019 01:49
@xxuejie xxuejie mentioned this pull request Jun 26, 2019
Merged
@xxuejie xxuejie force-pushed the xxuejie/nervosdao-refactor branch 2 times, most recently from 0a0adb5 to 15b9dc8 Compare June 26, 2019 07:53
@xxuejie
Copy link
Collaborator Author

xxuejie commented Jun 26, 2019

Right now RPC test still fails, will update the related hashes after #1119 is merged and we finished rebasing this PR.

@xxuejie xxuejie force-pushed the xxuejie/nervosdao-refactor branch from 5017e5d to f05f97c Compare June 27, 2019 06:03
@xxuejie xxuejie requested a review from doitian June 27, 2019 06:04
@xxuejie xxuejie force-pushed the xxuejie/nervosdao-refactor branch from cb80855 to 6f5545d Compare June 27, 2019 06:06
doitian
doitian requested changes Jun 27, 2019
View reviewed changes
util/dao/src/lib.rs Outdated Show resolved Hide resolved
@xxuejie xxuejie force-pushed the xxuejie/nervosdao-refactor branch from 245b39e to 6de273b Compare June 27, 2019 15:39
doitian
doitian previously approved these changes Jun 27, 2019
View reviewed changes
doitian
doitian reviewed Jun 27, 2019
View reviewed changes
util/dao/src/lib.rs Outdated

let (parent_ar, parent_c, parent_u) = extract_dao_data(parent.dao())?;

let parent_g2 = calculate_g2(
Copy link
Member

Choose a reason for hiding this comment

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

G1 and g2 are inconsistent. G1 uses the reward target of the parent block, while g2 uses the parent block.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is fixed now.

janx
janx reviewed Jun 28, 2019
View reviewed changes
Copy link
Member

@janx janx left a comment

Choose a reason for hiding this comment

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

The comments above ISSUING_DAO_HASH needs to be updated.

@janx
Copy link
Member

janx commented Jun 28, 2019

Do we still need the special ISSUING_DAO_HASH to identify a dao withdraw transaction? Can we check input type script instead?

@janx
Copy link
Member

janx commented Jun 28, 2019

Do we still need the special ISSUING_DAO_HASH to identify a dao withdraw transaction? Can we check input type script instead?

Yes we need that to issue the additional tokens.

Could you elaborate?

@xxuejie
Copy link
Collaborator Author

xxuejie commented Jun 28, 2019

Do we still need the special ISSUING_DAO_HASH to identify a dao withdraw transaction? Can we check input type script instead?

Yes we need that to issue the additional tokens.

Could you elaborate?

That's a mistake, second thought yes we can do that.

@xxuejie
Copy link
Collaborator Author

xxuejie commented Jun 28, 2019

@doitian @janx Issuing DAO input has been removed.

janx
janx reviewed Jun 28, 2019
View reviewed changes
util/dao/src/lib.rs Outdated Show resolved Hide resolved
janx
janx approved these changes Jun 28, 2019
View reviewed changes
Copy link
Member

@janx janx left a comment

Choose a reason for hiding this comment

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

I have no further comment for this large PR. Two improvements we should consider:

  • There is no clear way to get the amount g2 - miner_portion - dao_portion, which is the secondary issuance for treasury/governance in future.
  • An integration test to 1. generate occupied capacity changes randomly for several blocks, and 2. check the c in header.dao is consistent with occupied/deposited/free capacities in system.

@xxuejie xxuejie force-pushed the xxuejie/nervosdao-refactor branch from 25f90b7 to c9d0ace Compare June 28, 2019 04:34
@xxuejie
Copy link
Collaborator Author

xxuejie commented Jun 28, 2019

@nervos-bot-user try integration

@xxuejie xxuejie force-pushed the xxuejie/nervosdao-refactor branch from c9d0ace to 6a7002c Compare June 28, 2019 13:56
@xxuejie xxuejie merged commit be94f93 into develop Jun 28, 2019
@xxuejie xxuejie deleted the xxuejie/nervosdao-refactor branch June 28, 2019 14:39
zhangsoledad added a commit that referenced this pull request Jun 29, 2019
@zhangsoledad
Merge pull request #1134 from xxuejie/fix-integration-test
57fec2b 
fix: Integration test broken by #1094
@doitian doitian mentioned this pull request Jul 4, 2019
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhangsoledad zhangsoledad zhangsoledad approved these changes

@doitian doitian doitian approved these changes

@janx janx janx approved these changes

Assignees

@doitian doitian

Labels
b:consensus Break consensus b:database Break database schema breaking change The feature breaks consensus, database, message schema or RPC interface.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@xxuejie @doitian @janx @zhangsoledad