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(core-magistrate-transactions): ensure unique genesisHash per bridgechain #3199

Merged

Conversation

@Lemii
Copy link

Lemii commented Nov 4, 2019

Summary

This PR changes the following:

  • Implement validation that disallows duplicate genesisHash values for bridgechain registrations
  • Updates existing functional test to comply with new validation
  • Adds a new functional test that specifically checks for duplicate genesisHash values

To avoid multiple iterations over the same bridgechain array, I've refactored the check for duplicate bridgechain names to also accommodate for genesisHash validation.

For performance reasons a standard for loop is used. If a more clean and modern ES6 solution is preferred, we could go with something like the following as well:

        if (bridgechains) {
            Object.values(bridgechains).forEach(bridgechain => {
                if (bridgechain.bridgechainAsset.name === data.asset.bridgechainRegistration.name) {
                    throw new BridgechainAlreadyRegisteredError();
                }

                if (bridgechain.bridgechainAsset.genesisHash === data.asset.bridgechainRegistration.genesisHash) {
                    throw new GenesisHashAlreadyRegisteredError();
                }
            });
        }

Closes #3191

Checklist

  • Documentation (if necessary)
  • Tests (if necessary)
  • Ready to be merged
@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

ArkEcosystemBot commented Nov 4, 2019

Thanks for submitting this pull request! A maintainer will review this in the next few days and explicitly select labels so you know what's going on.

If no reviewer appears after a week, a reminder will be sent out.

@Lemii

This comment has been minimized.

Copy link
Author

Lemii commented Nov 4, 2019

codeclimate keeps failing with:

Function throwIfCannotBeApplied has a Cognitive Complexity of 11 (exceeds 5 allowed). Consider refactoring.

    public async throwIfCannotBeApplied(
        transaction: Interfaces.ITransaction,
        wallet: State.IWallet,
        walletManager: State.IWalletManager,
    ): Promise<void> {

I suppose I should just ignore this, considering I did not make any changes to that part of the code?

@Lemii Lemii requested a review from faustbrian Nov 4, 2019
@supaiku0

This comment has been minimized.

Copy link
Contributor

supaiku0 commented Nov 4, 2019

codeclimate keeps failing with:

Function throwIfCannotBeApplied has a Cognitive Complexity of 11 (exceeds 5 allowed). Consider refactoring.

    public async throwIfCannotBeApplied(
        transaction: Interfaces.ITransaction,
        wallet: State.IWallet,
        walletManager: State.IWalletManager,
    ): Promise<void> {

I suppose I should just ignore this, considering I did not make any changes to that part of the code?

Yes, that's out of scope.


I think checking whether a bridgechain name/genesis hash is unique or not has to be done globally, not locally per wallet.

I.e.

for (const wallet of walletManager.getIndex("businesses").values())
    const bridgechains = wallet.getAttribute("business.bridgechains");
    if (...)
faustbrian and others added 3 commits Nov 5, 2019
@Lemii

This comment has been minimized.

Copy link
Author

Lemii commented Nov 5, 2019

@supaiku0 I've implemented your request. It will now loop over all wallets, therefore checking for duplicate genesisHashes globally.

The bridgechain name check has not been included in this global check and has been reverted to its original form where it only checks the local wallet. The reason for this is that 'there can be multiple bridgechains with the same name on different businesses, because any further validation will be performed in the market place', which was mentioned to me by dated (thanks).

The duplicate genesisHash functional test has been adjusted to have bridgechains created from different businesses, therefore being more thorough.

faustbrian and others added 2 commits Nov 6, 2019
@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

ArkEcosystemBot commented Nov 7, 2019

A contributor has approved this PR. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait.

Thank you for your contribution!

@faustbrian faustbrian changed the title feat(core-magistrate-transactions): ensure unique genesisHash per bridgechain feat(core-magistrate-transactions): ensure unique genesisHash per bridgechain Nov 7, 2019
@faustbrian faustbrian merged commit 5dbc8a8 into ArkEcosystem:develop Nov 7, 2019
60 checks passed
60 checks passed
crypto (12.x)
Details
bridgechain-registration (12.x)
Details
unit (12.x)
Details
bridgechain-resignation (12.x)
Details
integration (12.x)
Details
bridgechain-update (12.x)
Details
e2e (12.x)
Details
business-registration (12.x)
Details
business-resignation (12.x)
Details
business-update (12.x)
Details
delegate-registration (12.x)
Details
delegate-resignation (12.x)
Details
htlc-claim (12.x)
Details
htlc-lock (12.x)
Details
htlc-refund (12.x)
Details
ipfs (12.x)
Details
multi-payment (12.x)
Details
multi-signature-registration (12.x)
Details
second-signature-registration (12.x)
Details
transfer (12.x)
Details
vote (12.x)
Details
LGTM analysis: JavaScript No new or fixed alerts
Details
codeclimate Approved by Brian Faust.
Details
security/snyk - __tests__/e2e/package.json (ArkEcosystem) No manifest changes detected
security/snyk - package.json (ArkEcosystem) No manifest changes detected
security/snyk - packages/core-api/package.json (ArkEcosystem) No manifest changes detected
security/snyk - packages/core-blockchain/package.json (ArkEcosystem) No manifest changes detected
security/snyk - packages/core-container/package.json (ArkEcosystem) No manifest changes detected
security/snyk - packages/core-database-postgres/package.json (ArkEcosystem) No manifest changes detected
security/snyk - packages/core-database/package.json (ArkEcosystem) No manifest changes detected
security/snyk - packages/core-elasticsearch/package.json (ArkEcosystem) No manifest changes detected
security/snyk - packages/core-error-tracker-airbrake/package.json (ArkEcosystem) No manifest changes detected
security/snyk - packages/core-error-tracker-bugsnag/package.json (ArkEcosystem) No manifest changes detected
security/snyk - packages/core-error-tracker-raygun/package.json (ArkEcosystem) No manifest changes detected
security/snyk - packages/core-error-tracker-rollbar/package.json (ArkEcosystem) No manifest changes detected
security/snyk - packages/core-error-tracker-sentry/package.json (ArkEcosystem) No manifest changes detected
security/snyk - packages/core-event-emitter/package.json (ArkEcosystem) No manifest changes detected
security/snyk - packages/core-exchange-json-rpc/package.json (ArkEcosystem) No manifest changes detected
security/snyk - packages/core-explorer/package.json (ArkEcosystem) No manifest changes detected
security/snyk - packages/core-forger/package.json (ArkEcosystem) No manifest changes detected
security/snyk - packages/core-http-utils/package.json (ArkEcosystem) No manifest changes detected
security/snyk - packages/core-interfaces/package.json (ArkEcosystem) No manifest changes detected
security/snyk - packages/core-jest-matchers/package.json (ArkEcosystem) No manifest changes detected
security/snyk - packages/core-logger-pino/package.json (ArkEcosystem) No manifest changes detected
security/snyk - packages/core-logger-signale/package.json (ArkEcosystem) No manifest changes detected
security/snyk - packages/core-logger-winston/package.json (ArkEcosystem) No manifest changes detected
security/snyk - packages/core-logger/package.json (ArkEcosystem) No manifest changes detected
security/snyk - packages/core-new-relic/package.json (ArkEcosystem) No manifest changes detected
security/snyk - packages/core-p2p/package.json (ArkEcosystem) No manifest changes detected
security/snyk - packages/core-snapshots/package.json (ArkEcosystem) No manifest changes detected
security/snyk - packages/core-state/package.json (ArkEcosystem) No manifest changes detected
security/snyk - packages/core-tester-cli/package.json (ArkEcosystem) No manifest changes detected
security/snyk - packages/core-transaction-pool/package.json (ArkEcosystem) No manifest changes detected
security/snyk - packages/core-transactions/package.json (ArkEcosystem) No manifest changes detected
security/snyk - packages/core-utils/package.json (ArkEcosystem) No manifest changes detected
security/snyk - packages/core-vote-report/package.json (ArkEcosystem) No manifest changes detected
security/snyk - packages/core-wallet-api/package.json (ArkEcosystem) No manifest changes detected
security/snyk - packages/core-webhooks/package.json (ArkEcosystem) No manifest changes detected
security/snyk - packages/core/package.json (ArkEcosystem) No manifest changes detected
security/snyk - packages/crypto/package.json (ArkEcosystem) No manifest changes detected
@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

ArkEcosystemBot commented Nov 7, 2019

Your pull request has been merged and marked as tier 3. It will earn you $50 USD.

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