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

feat: Implement Delegate Resignation (AIP11) #2538

Merged
merged 28 commits into from May 10, 2019

Conversation

Projects
None yet
5 participants
@vasild
Copy link
Contributor

commented May 7, 2019

No description provided.

@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

commented May 7, 2019

Your pull request doesn't follow our contribution guidelines. Please review and correct it.

@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

commented May 7, 2019

The ci/circleci: test-node10-functional job is failing as of eda71371b3a17945cb41bae9c5124c87dc8676ab. Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@supaiku0
Copy link
Contributor

left a comment

There's a fundamental flaw in how the resignation is implemented here. It just removes the delegate status of a wallet which in turn causes all it's current voters to end up voting for a non-delegate which breaks the protocol. Additionally, someone else could now take the delegate name which is primed to cause confusion.

Besides that, the implementation is basically completely different than outlined here:

The sender must be a delegate. Once forged the delegate is irreversibly deactivated:

  • no longer possible to vote for this delegate
  • no longer treated as an active delegates, regardless of vote balance

In other words, the username is not removed. Instead a flag should be set, indicating the delegate is inactive causing it to drop out the active delegates.

https://github.com/ArkEcosystem/core/blob/develop/packages/core-state/src/wallets/wallet-manager.ts#L131
https://github.com/ArkEcosystem/core/blob/develop/packages/core-state/src/wallets/wallet-manager.ts#L325

Furthermore, there are no functional tests, nor are the handler tests updated. For reference on where to find the relevant tests see the recently added IPFS type: #2537

I'm afraid the PR needs a complete overhaul. Thus I'm closing it for now.

@supaiku0 supaiku0 closed this May 7, 2019

vasild added some commits May 8, 2019

Merge remote-tracking branch 'ArkEcosystem/core/develop' into feat/de…
…legate-resignation

* ArkEcosystem/core/develop:
  chore(deps): update and remove unused dependencies (#2546)
  release: 2.4.0-next.2 (#2545)
  feat(core-wallet-api): initial implementation (#2544)
  fix(core): use the --env flag to start testnet via CLI (#2543)
  release: 2.4.0-next.1 (#2541)
  refactor(core-database-postgres): transaction type agnostic wallet bootstrap (#2539)
  release: 2.4.0-next.0 (#2540)
  refactor(core): let the process manager extend @faustbrian/foreman (#2536)
  feat: Implement IPFS (AIP11) (#2537)
Merge remote-tracking branch 'ArkEcosystem/core/develop' into feat/de…
…legate-resignation

* ArkEcosystem/core/develop:
  release: 2.4.0-next.3 (#2549)
  refactor(core-p2p): reduce log noise (#2548)
  fix(core-p2p): ignore maxSameSubnetPeers in seed mode (#2547)

@vasild vasild reopened this May 8, 2019

@faustbrian
Copy link
Collaborator

left a comment

Needs a revert and removal of unnecessary data.

@vasild

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

Alright, I added a resigned flag to mark delegates that have resigned instead of un-delegating them.

The sender must be a delegate

The check is here: https://github.com/ArkEcosystem/core/pull/2538/files#diff-304db985d999842253be956ceec47a2aR20 !wallet.username

Once forged the delegate is irreversibly deactivated

Will be handled by existing code: https://github.com/ArkEcosystem/core/blob/afb69f2141383/packages/core-transactions/src/handlers/delegate-registration.ts#L60

no longer possible to vote for this delegate

Added a check here: https://github.com/ArkEcosystem/core/pull/2538/files#diff-ced27755786a5f642370021ba08a2853R66

no longer treated as an active delegates, regardless of vote balance

Removed resigned delegates: https://github.com/ArkEcosystem/core/pull/2538/files#diff-097e9ec56428de146b54cea1f67d0448R325

@supaiku0
Copy link
Contributor

left a comment

Left some comments. The resignation transaction builder also needs to set version 2, otherwise they can't be verified and not tested (necessary when writing the tests).

Once done, we need a few functional tests and the handler tests also need to be updated (use the other types for reference).

See:
https://github.com/ArkEcosystem/core/blob/develop/__tests__/unit/core-transactions/handler.test.ts#L802
https://github.com/ArkEcosystem/core/tree/develop/__tests__/functional/transaction-forging

@@ -129,19 +129,16 @@ export class WalletManager implements State.IWalletManager {
}

public loadActiveDelegateList(roundInfo: Shared.IRoundInfo): State.IDelegateWallet[] {
const delegates: State.IWallet[] = this.buildDelegateRanking(undefined, roundInfo);

This comment has been minimized.

Copy link
@supaiku0

supaiku0 May 8, 2019

Contributor

Why this change? Line 325 should be enough.

This comment has been minimized.

Copy link
@vasild

vasild May 9, 2019

Author Contributor

This change is because we want to check against maxDelegates with resigned delegates removed. Thus need to call buildDelegateRanking() before the check because allByUsername() would not filter anything.

Also this change makes the code simpler because it eliminates the call to allByUsername() and removes the variable delegatesWallets, so one call and one variable less.

Btw what a wretched naming for maxDelegates! We actually want the number of delegates to be bigger than that and emit an error if it is less. minDelegates would be more appropriate, but out of the scope of this PR.

Show resolved Hide resolved packages/core-state/src/wallets/wallet.ts
Show resolved Hide resolved packages/core-transactions/src/errors.ts Outdated
Show resolved Hide resolved packages/core-transactions/src/handlers/delegate-resignation.ts Outdated
Show resolved Hide resolved packages/core-transactions/src/handlers/delegate-resignation.ts Outdated
Show resolved Hide resolved ...es/crypto/src/transactions/builders/transactions/delegate-resignation.ts Outdated
Show resolved Hide resolved ...es/crypto/src/transactions/builders/transactions/delegate-resignation.ts Outdated
Show resolved Hide resolved packages/crypto/src/transactions/serializer.ts Outdated
Show resolved Hide resolved packages/crypto/src/transactions/types/delegate-resignation.ts Outdated
Show resolved Hide resolved packages/crypto/src/transactions/types/delegate-resignation.ts Outdated

faustbrian added some commits May 9, 2019

@faustbrian faustbrian changed the title feat: complete the delegate resignation transaction type feat: Implement Delegate Resignation (AIP11) May 9, 2019

@codecov-io

This comment has been minimized.

Copy link

commented May 9, 2019

Codecov Report

Merging #2538 into develop will increase coverage by 0.01%.
The diff coverage is 46.15%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2538      +/-   ##
===========================================
+ Coverage    66.82%   66.83%   +0.01%     
===========================================
  Files          406      406              
  Lines         8864     8882      +18     
  Branches       460      418      -42     
===========================================
+ Hits          5923     5936      +13     
- Misses        2886     2893       +7     
+ Partials        55       53       -2
Impacted Files Coverage Δ
...rs/src/transactions/types/delegate-registration.ts 100% <ø> (ø)
...ions/builders/transactions/delegate-resignation.ts 100% <100%> (+26.66%) ⬆️
packages/core-state/src/wallets/wallet-manager.ts 73.1% <100%> (-0.19%) ⬇️
packages/core-state/src/wallets/wallet.ts 88.15% <100%> (+0.15%) ⬆️
...-transactions/src/handlers/delegate-resignation.ts 33.33% <15.38%> (-11.12%) ⬇️
packages/core-transactions/src/errors.ts 81.81% <50%> (-0.54%) ⬇️
packages/core-transactions/src/handlers/vote.ts 66.66% <66.66%> (-0.99%) ⬇️
packages/core-logger-winston/src/formatter.ts 41.66% <0%> (ø) ⬆️
...es/core-blockchain/src/replay/replay-blockchain.ts 0% <0%> (ø) ⬆️
...pto/src/transactions/types/delegate-resignation.ts 100% <0%> (+22.22%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f6b217...805c3d8. Read the comment docs.

@faustbrian faustbrian self-requested a review May 9, 2019

@faustbrian faustbrian requested a review from supaiku0 May 9, 2019

vasild added some commits May 9, 2019

chore: remove `delegates` argument from buildDelegateRanking()
The only caller that was providing the optional argument `delegates` was
supplying the same thing as what buildDelegateRanking() would use
internally if no argument was given.
Merge remote-tracking branch 'ArkEcosystem/core/develop' into feat/de…
…legate-resignation

* ArkEcosystem/core/develop:
  feat(core-wallets-api): proxy API calls to core-api until fully developed (#2558)
test: use unitnet for unit tests
unitnet has aip11 enabled, so that it can process delegate resignation
transactions, which are version 2.
@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

commented May 10, 2019

Your pull request needs some changes. Please wait for a comment from one of our developers for more information.

faustbrian and others added some commits May 10, 2019

Merge remote-tracking branch 'ArkEcosystem/core/develop' into feat/de…
…legate-resignation

* ArkEcosystem/core/develop:
  feat(core): add database configuration command (#2557)

@faustbrian faustbrian requested a review from supaiku0 May 10, 2019

faustbrian and others added some commits May 10, 2019

@faustbrian faustbrian merged commit 1c30978 into develop May 10, 2019

12 checks passed

ci/circleci: test-node10-benchmark Your tests passed on CircleCI!
Details
ci/circleci: test-node10-e2e Your tests passed on CircleCI!
Details
ci/circleci: test-node10-functional Your tests passed on CircleCI!
Details
ci/circleci: test-node10-integration-0 Your tests passed on CircleCI!
Details
ci/circleci: test-node10-integration-1 Your tests passed on CircleCI!
Details
ci/circleci: test-node10-unit Your tests passed on CircleCI!
Details
ci/circleci: test-node11-benchmark Your tests passed on CircleCI!
Details
ci/circleci: test-node11-e2e Your tests passed on CircleCI!
Details
ci/circleci: test-node11-functional Your tests passed on CircleCI!
Details
ci/circleci: test-node11-integration-0 Your tests passed on CircleCI!
Details
ci/circleci: test-node11-integration-1 Your tests passed on CircleCI!
Details
ci/circleci: test-node11-unit Your tests passed on CircleCI!
Details

@ArkEcosystemBot ArkEcosystemBot deleted the feat/delegate-resignation branch May 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.