neo-project / neo

NEO Smart Economy
MIT License
3.47k stars 1.03k forks source link

NEO Core Developer Work Guidelines (Draft) #2955

Open Jim8y opened 1 year ago

Jim8y commented 1 year ago

this is from @lock9, i think he presents the guideline better than me.

Development Process Guidelines

  1. Voluntary Issue Assignment:

    • Core developers may voluntarily take on new issues.
    • They must comment on the issue to claim it.
    • If multiple developers are interested in the same issue, priority goes to the developer who created the issue, followed by the solution proposer.
  2. Issue Discussion Before PR:

    • Core developers should thoroughly discuss their proposed solution on the issue thread before starting work.
    • The team must approve the issue before proceeding to PR.
    • An issue must be closed with a reason (@roman-khimov).
  3. Descriptive PR Submissions:

    • PRs (Pull Requests) should clearly outline the problem and the proposed solution.
  4. Solution Voting on Disagreements:

    • All core developers will vote in cases of differing solutions for the same problem.
    • A simple majority decides the approach.
    • Any core dev can call a vote on a decision.
    • Voting must happen within 21 days.
    • The PR submission follows the chosen solution.
  5. Voting on Self-Submitted Issues:

    • Core developers can work on PRs for their own submitted issues.
    • These issues must be approved by a core developer vote.
  6. Comprehensive Comments and Updates:

    • Submitted code should include inline comments and unit tests.
    • PRs must not decrease overall code coverage.
  7. Reporting Breaking Changes:

    • Changes affecting other SDKs or Neo's general behavior must be communicated.
    • Developers should provide necessary update instructions.
  8. Draft Mode for PRs:

    • PRs should be marked as draft until they are ready for merge.
    • Ready-for-merge PRs require voluntary reviews from multiple core developers.
  9. Timely PR Reviews:

    • If a PR lacks sufficient reviews within one month, it will be considered stalled.
    • Stalled PRs are considered incidents and will require action from the team.

bellow is the old version.

NEO Core Developer Work Guidelines

  1. Voluntarily Taking on Issues:

When new issues arise, core developers can voluntarily take on the task of resolving them.

  1. Issue Discussion Before PR:

Before submitting a pull request (PR), core developers must describe their proposed solutions in detail within the issue thread.

  1. Descriptive PR Submissions:

PRs must clearly describe the problem being solved and the solution.

  1. Solution Voting on Disagreements:

If multiple core developers propose different solutions to the same problem, all core developers will collectively vote on which approach to take using a simple majority vote. The PR is only submitted after a decision is made.

  1. Voting on Self-Submitted Issues:

Core developers can work on PRs for issues they submit, but the issues must be approved by a core developer vote.

  1. Comprehensive Comments and Updates:

Code submitted by core developers should contain detailed code comments. Related documentation, examples, and unit tests should also be updated to reflect changes in the code. And pr should never decrease the code coverage. This ensures the codebase maintains good records and accessibility for other developers, and consistency of features gets tested and verified.

  1. Termination for Prolonged Inactivity:

Core developers who do not participate in reviews and votes for a prolonged period may be removed from their role.

  1. Draft Mode for PRs:

PRs must be set to draft mode until ready for merge. PRs ready for merge require voluntary reviews from several other core developers.

  1. Forced Reviews on Stagnant PRs (non-plan prs):

If a PR does not get enough reviews within one month, all core developers must forcibly participate in the review process.

  1. Activity-Based Incentives:

Incentives for core developers are determined by their activity levels, managed by NGD. Factors like voting, reviewing, submitting, opening issues and PRs, and participating in discussions may contribute to activity metrics.

  1. Respecting Voting Outcomes:

Core developers must respect and adhere to the outcomes of the voting process.

vncoelho commented 1 year ago

The rest is ok, @Liaojinghui. But, honestly, I think it is still a little far from a guidelines I would like to see. Too much things here you are trying to impose. The guidelines should be more focused on good practices and also be aligned with better practices for testing and a better organization of the project.

In particular, I think that 2 and 3 are the best ones in your description. You should go in this direction I think.

Jim8y commented 1 year ago

@vncoelho I did not make this list, its a collection from the neo ecosystem. Something we can discuss, but something we have to put there, not up to me, lets wait for other's opinions. Exit and force review is necessary to make sure that no prs get unreviewed for years (some prs waited for 2.5 years), and is not up to me to remove them.

Again, i do not make any decision here, i am just someone put every suggestion together.

New core developers will be added, we can vote on this.

csmuller commented 1 year ago

@vncoelho I think these guidelines are not only about how to approach work as a core developer but also how to organize the core developers and the processes they use to work together. Thus, points like 7 or 9 are not absurd in my opinion.

@Liaojinghui, for the points that contain voting I would make it clear how the voting works. E.g.

4. Solution Voting on Disagreements: If multiple core developers propose different solutions to the same problem, all core developers will collectively vote on which approach to take using a simple majority vote. The PR is only submitted after a decision is made.

shargon commented 1 year ago

7- I think that is needed. 9- Oracles spend more than a month working active. 10- In general, it is not necessary to encourage people to work, they need a good, fixed and stable salary; If incentives are not transparent, incentives are worst, and if someone doesn't work we can resort to point 7. With a fixed salary, there won't be fights between core developers, just they will work as team, that is what we need.

Jim8y commented 1 year ago

9- Oracles spend more than a month working active.

one month for review, not for development.

fixed salary.

personally not against it. But some sort of encouragment is necessary. Companies do this for a reason, a reason of humanity. No fund for developers, no boost for developers, no grand for developers, but all blame developers, you can do it, i can do it, but how about other developers? how about attracting more brilliant developers? Without the best developers, without the best project.

vncoelho commented 1 year ago

I agree with what you mentioned about point 10, @shargon. Incentives, in the past, were worst. It looks like a good, fixed and stable reward/salary will be enough to keep the current developers and attract news ones. There can be levels of rewards/salary according to experience and contributions, off course. That was in line with some recent suggestions I recently sent to Neo Foundation.

I still think that, from that list, what is really important are points 2) and 3). If we go in that direction we will already improve a lot experience, because PRs will be opened after a previous agreement on the issue and precise descriptions.

shargon commented 1 year ago

9- Oracles spend more than a month working active.

one month for review, not for development.

fixed salary.

personally not against it. But some sort of encouragment is necessary. Companies do this for a reason, a reason of humanity.

Could you give me one example of a company who pay by individual incentives the gross of the income? Bonus is different than incentives and we are not a salesperson 😅.

Jim8y commented 1 year ago

Could you give me one example of a company who pay by individual incentives the gross of the income? Bonus is different than incentives and we are not a salesperson 😅.

Then please tell me how are we evaluate the performance of developers? Every one get the same even by doing nothing? A communist society? I think all company have KPI (or other type of evaluations) to rank the performance of developers.

Again, this is not about you, not about me, not about vitor, or roman, it is about the system whether it can attract good developers and treat them fairly. We can ask neo to pay everyone of us 30K USD per month, then everyone will be happy and you will do your job, but what about those who dont work at all? Or just do very small amount of work? It is possible in the future when we have other core developer. Its a system thing.

I can understand how much shargon love neo and is willing to contribute, but you should not expect others do the same.

lock9 commented 1 year ago

Hi,

Voluntarily Taking on Issues: When new issues arise, core developers can voluntarily take on the task of resolving them.

What does 'taking' mean? How is task distribution going to work?

Issue Discussion Before PR: Before submitting a pull request (PR), core developers must describe their proposed solutions in detail within the issue thread.

Ok. How do they know it has been decided?

Descriptive PR Submissions: PRs must clearly describe the problem being solved and the solution.

There must be a template for this or some way to measure if the PR has enough information about it. Also, shouldn't this information be on the issue, and not on the PR?

Solution Voting on Disagreements: If multiple core developers propose different solutions to the same problem, all core developers will collectively vote on which approach to take using a simple majority vote. The PR is only submitted after a decision is made.

What configures a disagreement? A thumbs-down?

Voting on Self-Submitted Issues: Core developers can work on PRs for issues they submit, but the issues must be approved by a core developer vote.

Ok. What defines approval? How is the discovery and planning process for the core devs?

Comprehensive Comments and Updates: Code submitted by core developers should contain detailed code comments. Related documentation, examples, and unit tests should also be updated to reflect changes in the code. And pr should never decrease the code coverage. This ensures the codebase maintains good records and accessibility for other developers, and consistency of features gets tested and verified.

By documentation, you are referring to inline docs, right? I don't think devs will create external documentation.

Termination for Prolonged Inactivity: Core developers who do not participate in reviews and votes for a prolonged period may be removed from their role.

We should have an alumni program for ex-contributors. They should not be just 'terminated and excluded' but transformed into former collaborators.

Draft Mode for PRs: PRs must be set to draft mode until ready for merge. PRs ready for merge require voluntary reviews from several other core developers.

Ok

Forced Reviews on Stagnant PRs (non-plan prs): If a PR does not get enough reviews within one month, all core developers must forcibly participate in the review process.

One month is too much. Can we start with 1 month and plan to reduce it to 14 or even 7 days? Actually, can we make it this forced review just on PRs that are inactive? (no comments or updates). It doesn't make sense to force a review on something is being discussed

Activity-Based Incentives: Incentives for core developers are determined by their activity levels, managed by NGD. Factors like voting, reviewing, submitting, opening issues and PRs, and participating in discussions may contribute to activity metrics.

I suggest that incentives increase with constant activity. For example: If you contribute every week, you have right to '100%' of the reward. If you contribute only just a month, you have the right to only 30%—something like that. We need to ensure developers are coming back daily/weekly. I think core devs should run a consensus node and split the reward. Maybe 1 consensus node for every 2 - 4 devs? This will force a relationship. I'm not sure how this could work, but being rewarded an extra amount in GAS will make devs focus on delivering value instead of just PRs.

Respecting Voting Outcomes: Core developers must respect and adhere to the outcomes of the voting process.

Ok

shargon commented 1 year ago

what about those who dont work at all?

FIRED 🤷‍♂️. If you want to attract talent, talent require stability.

Jim8y commented 1 year ago

@lock9

What does 'taking' mean? How is task distribution going to work?

core developers who wish to address the issue can ask for it, post his solution. if no other core developers ask for it, he can work on it. Simple issues, a few lines to fix as shargon mentioned, can be directly fixed by the core developer who take it first.

Ok. How do they know it has been decided?

we vote, we count, the vote result is public.

There must be a template for this or some way to measure if the PR has enough information about it. Also, shouldn't this information be on the issue, and not on the PR?

issue can be simple since it just describe the general idea. Pr will be more detailed since it is realated to implementation. Asking core developers to have a detailed design on an issue will consume too much time since things offen change during the implementation.

What configures a disagreement? A thumbs-down?

This can be decided, thumbs up and a heart for example.

Ok. What defines approval? How is the discovery and planning process for the core devs?

vote defines approval.

By documentation, you are referring to inline docs, right? I don't think devs will create external documentation.

I am considering adding document to the monorepo, we must keep the document up to data. I dont want to update document neither, but we must do it for the good of neo.

We should have an alumni program for ex-contributors. They should not be just 'terminated and excluded' but transformed into former collaborators.

this is not up to us, maybe join the technical committee?

One month is too much. Can we start with 1 month and plan to reduce it to 14 or even 7 days?

Some prs contain too much to review, 1 month for those large prs, 7 days for small prs is reasonable.

I suggest that incentives increase with constant activity. For example: If you contribute every week, you have right to '100%' of the reward. If you contribute only just a month, you have the right to only 30%—something like that. We need to ensure developers are coming back daily/weekly. I think core devs should run a consensus node and split the reward. Maybe 1 consensus node for every 2 - 4 devs? This will force a relationship. I'm not sure how this could work, but being rewarded an extra amount in GAS will make devs focus on delivering value instead of just PRs.

We are still discussing on this. But this actually not up to us as well.

Jim8y commented 1 year ago

what about those who dont work at all?

FIRED 🤷‍♂️. If you want to attract talent, talent require stability.

Stability, in the mean while incentive, who will reject? To be honest, stable salary for a web3 talent would require a lot of money. Just check the average salary for a Web3 developer in USA, neo will go bankrupt if all get paid "stability"

shargon commented 1 year ago

what about those who dont work at all?

FIRED 🤷‍♂️. If you want to attract talent, talent require stability.

Stability, in the mean while incentive, who will reject? To be honest, stability salary for a web3 talent would require a lot of money. Just check the average salary for a Web3 developer in USA, neo will go bankrupt if all get paid "stability"

Then don't expect them if they are your target when you say "attract talent" 🤣.

If you want to be full-time in something you need to pay your bills with that, and incentives sometimes are no related with your work at all, one line could spend 2 minutes or one week. And also, after this week, another core developer was faster and push his version before you, then you will be frustrated and at the end you will leave it. Stability is mandatory.

Jim8y commented 1 year ago

If you want to be full-time in something you need to pay your bills with that, and incentives sometimes are no related with your work at all, one line could spend 2 minutes or one week. And also, after this week, another core developer was faster and push his version before you, then you will be frustrated and at the end you will leave it. Stability is mandatory.

How to have a fair incentive plan is another question than Whether should we have an incentive plan.

Please consider others as ordinary human beings, not someone like you who loves neo. I know you will work full time on neo if paid decently, but, can neo core maintaining rely on your own? No other core developers are needed? What we need is a system, not a personal love, a system that forces (encourages) people to do their work.

lock9 commented 1 year ago

What if it is: Base Salary + Rewards + Consensus Node?

We are not salespeople, but we must reward people by performance/results. Otherwise, the system may become unfair, moving people away from Neo.

shargon commented 1 year ago

Base Salary + Rewards + Consensus Node

Looks good to me. But consensus node must be voted by people...

steven1227 commented 1 year ago
  • Remove 7
  • obviously remove 9 (what is this @Liaojinghui : all core developers must forcibly participate in the review process.". Must forcibly participate? This is absurd, Jimmy.
  • Remove 10, this is not related here at all.

The rest is ok, @Liaojinghui. But, honestly, I think it is still a little far from a guidelines I would like to see. Too much things here you are trying to impose. The guidelines should be more focused on good practices and also be aligned with better practices for testing and a better organization of the project.

In particular, I think that 2 and 3 are the best ones in your description. You should go in this direction I think.

7 is necessary in my mind. We should have a punishment mechanism to push people to work.

vncoelho commented 1 year ago

Push people to work,@steven1227? I think that it is not a good way to go. I believe that @erikzhang never tried to attract Core Devs in that direction.

Jim8y commented 1 year ago

well, if you still have no idea, neo core is almost a dead project, almost no one is using it. if we keep it this way, it will be dead for sure. issues and prs left there for years, never heard in any other projects, all of us are to blame.

csmuller commented 1 year ago

Base Salary Rewards/Bonuses Punishment

Depending on how complicated we want the financial incentive structure to be, we can include all of the above into it.

shargon commented 1 year ago

@lock9 Consensus Node is a good idea, but... it can't be done with a multisignature, we can't have a consensus node that require to sign the block by different wallets, so it will be owned by only one, also it require voting power. But I think it is not normal for a single consensus node to earn more than all the core developers combined.

Jim8y commented 1 year ago

I really dont want to disappoint anyone here, but incentive or base salary actually is not up to us. All consensus nodes are already distributed among communities and every commuity has a good reason to keep it, it is hard to get one for the core devs.

lock9 commented 1 year ago

Hello. I've noticed that the conversation has moved away from the central topic. Let's discuss items 7, 9, and 10 in a different issue. There are disagreements only on those items. Item 6 is too big and could be divided into smaller items for better clarification. Please let me know if you guys agree about the other items.

Note: The process is still incomplete. We don't mention discovery, planning, and testing activities, but let's try to do this in small steps.

You guys still need to decide:

Important: I can tell you that @Liaojinghui is doing the work as a developer and product owner. Developers want to code, not make plans. He is going to feel exhausted soon.

lock9 commented 1 year ago

The problem with the issue, as it is, is that we are discussing processes and governance together. Some processes are related to governance, but not all. So we could narrow down this issue to only discussions around the development process. Voting on issues is part of the process, so I would leave it here. However, compensation, termination, and the consequences of not doing 'mandatory' activities should be decided elsewhere.

lock9 commented 1 year ago

I made some changes to the original proposal. What do you guys think?

Development Process Guidelines

  1. Voluntary Issue Assignment:

    • Core developers may voluntarily take on new issues.
    • They must comment on the issue to claim it.
    • If multiple developers are interested in the same issue, priority goes to the developer who created the issue, followed by the solution proposer.
  2. Issue Discussion Before PR:

    • Core developers should thoroughly discuss their proposed solution on the issue thread before starting work.
    • The team must approve the issue before proceeding to PR.
  3. Descriptive PR Submissions:

    • PRs (Pull Requests) should clearly outline the problem and the proposed solution.
  4. Solution Voting on Disagreements:

    • All core developers will vote in cases of differing solutions for the same problem.
    • A simple majority decides the approach.
    • Any core dev can call a vote on a decision.
    • Voting must happen within 21 days.
    • The PR submission follows the chosen solution.
  5. Voting on Self-Submitted Issues:

    • Core developers can work on PRs for their own submitted issues.
    • These issues must be approved by a core developer vote.
  6. Comprehensive Comments and Updates:

    • Submitted code should include inline comments and unit tests.
    • PRs must not decrease overall code coverage.
  7. Reporting Breaking Changes:

    • Changes affecting other SDKs or Neo's general behavior must be communicated.
    • Developers should provide necessary update instructions.
  8. Draft Mode for PRs:

    • PRs should be marked as draft until they are ready for merge.
    • Ready-for-merge PRs require voluntary reviews from multiple core developers.
  9. Timely PR Reviews:

    • If a PR lacks sufficient reviews within one month, it will be considered stalled.
    • Stalled PRs are considered incidents and will require action from the team.
Jim8y commented 1 year ago

@lock9 much better than my version

Jim8y commented 1 year ago

Important: I can tell you that @Liaojinghui is doing the work as a developer and product owner. Developers want to code, not make plans. He is going to feel exhausted soon.

Thank you very much @lock9 for your kind caring, its very nice of you. Things like this wont last for long, as lone as we have a solid plan for everything, we nolonger need to worry about them.

As to the vote,

Jim8y commented 1 year ago

@csmuller @shargon @vncoelho @steven1227 @lock9, when i discuss the incentive with edge, i came up with an idea that might be able to address all of our concerns. The neo-eco can donate coredevs seperately.

core-devs can have a fixed salary, but in the meanwhile, it should be the community to decide whether a coredev deserve a bonus by donating him privately. Consensus candidates gets a lot of gas, they can decide by themselves whether they want to give a small portion of their incomes to the coredev team seperately and privately, totally by themselves and no one should blame anything. We can setup a coredev donation website that statisticly shows our information, our activity, our donation address.

roman-khimov commented 1 year ago

N. Closing an issue requires core dev consensus. If an issue is closed manually, there MUST be a comment describing how and where it's fixed or why it shouldn't/can't be fixed.

A bit similar to https://github.com/nspcc-dev/.github/blob/master/project-management.md#issue-closure

Jim8y commented 1 year ago

N. Closing an issue requires core dev consensus. If an issue is closed manually, there MUST be a comment describing how and where it's fixed or why it shouldn't/can't be fixed.

A bit similar to https://github.com/nspcc-dev/.github/blob/master/project-management.md#issue-closure

Yes for the future issues, but i think it would be a waste of time to check one by one in detail and ask for a reason for issues that are 4 or 5 years old. If someone is going to fix it, it should have already being fixed. Some of the issues opened for neo 2.0 and discussion of those issues stop for at least 3 years.

So, a reason of "inactive for too long" would be given the next time i close those issues (4 years old issues only i promise).

a workflow of "close stale issues" maybe

name: "Close Stale Issues"

on:
  schedule:
    - cron: '0 0 * * *' # Runs once every day

jobs:
  stale:
    runs-on: ubuntu-latest
    steps:
    - uses: actions/checkout@v2
    - name: Mark stale issues and pull requests
      uses: actions/stale@v3
      with:
        repo-token: ${{ secrets.GITHUB_TOKEN }}
        stale-issue-message: 'This issue has been inactive for 30 days. It will be automatically closed in 10 days if no further activity occurs.'
        stale-pr-message: 'This pull request has been inactive for 30 days. It will be automatically closed in 10 days if no further activity occurs.'
        days-before-stale: 30
        days-before-close: 10
        stale-issue-label: 'stale'
        stale-pr-label: 'stale'
shargon commented 1 year ago

N. Closing an issue requires core dev consensus. If an issue is closed manually, there MUST be a comment describing how and where it's fixed or why it shouldn't/can't be fixed. A bit similar to https://github.com/nspcc-dev/.github/blob/master/project-management.md#issue-closure

Yes for the future issues, but i think it would be a waste of time to check one by one in detail and ask for a reason for issues that are 4 or 5 years old. If someone is going to fix it, it should have already being fixed. Some of the issues opened for neo 2.0 and discussion of those issues stop for at least 3 years.

So, a reason of "inactive for too long" would be given the next time i close those issues (4 years old issues only i promise).

a workflow of "close stale issues" maybe

name: "Close Stale Issues"

on:
  schedule:
    - cron: '0 0 * * *' # Runs once every day

jobs:
  stale:
    runs-on: ubuntu-latest
    steps:
    - uses: actions/checkout@v2
    - name: Mark stale issues and pull requests
      uses: actions/stale@v3
      with:
        repo-token: ${{ secrets.GITHUB_TOKEN }}
        stale-issue-message: 'This issue has been inactive for 30 days. It will be automatically closed in 10 days if no further activity occurs.'
        stale-pr-message: 'This pull request has been inactive for 30 days. It will be automatically closed in 10 days if no further activity occurs.'
        days-before-stale: 30
        days-before-close: 10
        stale-issue-label: 'stale'
        stale-pr-label: 'stale'

Those old issues doesn't hurt us, we should close them with a valid reason

Jim8y commented 1 year ago

Isn't 3 years of no one touching a good reason to close an issue? I dont understand, you keep it open yet you dont care (many years no one follow), and you dont want it closed,,,,,,, I think an issue that no one follows for over 3 years already means its not accepted.

And old issues do hurt us, cause we should check them one by one to make sure we dont miss valuable issues that contain bug reports and feature requests. For those already proved of no interest, we should close them, instead of leave them for another 4 years.

lock9 commented 1 year ago

I don't think we should close it. I think we should review all of them. Most of them can be closed, maybe 70%, but there is a lot of important feedback that shouldn't be forgotten. Some issues on Neo Express and the Debugger are years old but still relevant.

From https://github.com/neo-project/neo/issues/2949:

Review of Older Issues and PRs: Go through over 300 open issues and PRs across different repositories to address the feeling of abandonment and ensure that important feedback is not missed.

Let's not close them, please. I can prepare a report so we can do this quickly

Jim8y commented 1 year ago

I don't think we should close it. I think we should review all of them. Most of them can be closed, maybe 70%, but there is a lot of important feedback that shouldn't be forgotten. Some issues on Neo Express and the Debugger are years old but still relevant.

@lock9 that is exactly why i wish to close old and not important issues. We need to close "70%" unimportant, then focus on "30%" that are important. going through 300 issues is not as easy as saying it, may take many years if we have to rediscuss them again. So, what i prefer to do is close those that looks unimportant, then reopen those that are miss closed.

lock9 commented 1 year ago

I understand your concern:

may take many years

However:

Jim8y commented 1 year ago

Then why not just close them, then reopen on demands such that everyone who are involved can join the process and save us time on those old old old issues. Anyway, other coredevs also prefer your way, so I don't want to argue, I will always respect the majority.

We may always have disputes, no need to convince anyone, just respect the majority. Let's focus on fixing issues.

lock9 commented 1 year ago

Hi everyone. Please check #2969 I've created a PR so you can comment on the file, change it, or send a PR with changes.

You can read it easier here: https://github.com/lock9/neo/blob/patch-1/Governance.md

lock9 commented 1 year ago

Then why not just close them, then reopen on demands such that everyone who are involved can join the process and save us time on those old old old issues. Anyway, other coredevs also prefer your way, so I don't want to argue, I will always respect the majority. We may always have disputes, no need to convince anyone, just respect the majority. Let's focus on fixing issues.

The problem is that some people like @roman-khimov and @shargon think we should only close it with a reason.

Let's try to streamline the voting process: go into the issues and instead of closing, add your vote following something like this:

👍 : +1 - Keep it open, add to the backlog 👎 : -1 - Close it 👀 : 0 - You don't have an opinion, but you have cast your vote

lock9 commented 1 year ago

I've tried doing this for 10 minutes and regretted it. I think we need some initial work to be done. There are a lot of good discussions that should be considered.

Jim8y commented 1 year ago

I think we should have agreed with @shargon and @roman-khimov to not close issues without a good reason for those ancient issues. So we can move on this way.