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

chore: debrand core by renaming ARK to CORE #1970

Merged
merged 12 commits into from
Jan 16, 2019
Merged

chore: debrand core by renaming ARK to CORE #1970

merged 12 commits into from
Jan 16, 2019

Conversation

faustbrian
Copy link
Contributor

Proposed changes

This PR debrands core by renaming all instances of ARK to CORE and the API vendor from ark.core-api to core-api.

This makes core more generic and easier for sidechains to deploy as they don't need to rename anything, simply generate your custom network config and you are ready to go.

Resolves #1965

Types of changes

  • Other

Checklist

  • I have read the CONTRIBUTING documentation
  • Lint and unit tests pass locally with my changes

@codecov-io
Copy link

codecov-io commented Jan 14, 2019

Codecov Report

Merging #1970 into develop will decrease coverage by 0.25%.
The diff coverage is 20.75%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1970      +/-   ##
===========================================
- Coverage    38.91%   38.66%   -0.26%     
===========================================
  Files          376      385       +9     
  Lines         8215     8279      +64     
  Branches      1150     1184      +34     
===========================================
+ Hits          3197     3201       +4     
- Misses        5006     5066      +60     
  Partials        12       12
Impacted Files Coverage Δ
...p2p/src/server/versions/internal/schemas/blocks.ts 0% <ø> (ø) ⬆️
...src/validation/extensions/transactions/transfer.ts 100% <ø> (ø) ⬆️
...s/crypto/src/builder/transactions/multi-payment.ts 76.19% <ø> (ø) ⬆️
...ation/extensions/transactions/timelock-transfer.ts 100% <ø> (ø) ⬆️
...ckages/core-jest-matchers/src/fields/public-key.ts 100% <ø> (ø) ⬆️
...pto/src/validation/extensions/transaction-array.ts 100% <ø> (ø) ⬆️
...rypto/src/builder/transactions/second-signature.ts 100% <ø> (ø) ⬆️
...pto/src/validation/extensions/transactions/ipfs.ts 100% <ø> (ø) ⬆️
...ckages/crypto/src/validation/extensions/address.ts 100% <ø> (ø) ⬆️
packages/crypto/src/identities/keys.ts 100% <ø> (ø) ⬆️
... and 73 more

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 795c077...8db13f3. Read the comment docs.

@geopsllc
Copy link

Since 2.1 already changes config structure, it will make sense to also change the data folder now as mentioned below. There are still quite a few more things that need debranding but this is a good start.

  1. Default USER from "ark" to "core"
  2. CORE_DB_DATABASE from ark_${process.env.CORE_NETWORK_NAME} to core_${process.env.CORE_NETWORK_NAME}
  3. Data folder from .ark/ to .core/
  4. All the comments from Ark to Core
  5. toBeArkAddress, arkBlock, arkAddress, arkBlockId, arkTransactionArray, arkPublicKey, arkTransfer, arkSecondSignature, arkDelegateRegistration, arkVote, arkMultiSignature, arkUsername, arkDelegateResignation, arkIpfs, arkMultiPayment, arkTimelockTransfer, ark-codec, ArkCodec, arkCrypto, arkEncoders, ark-development, ark-core, ark-devnet, ark-mainnet, ark-testnet
  6. module core-snapshots is riddled with "ark" everywhere, including filenames.
  7. Maybe also rebrand arkjs to corejs - not a must but will make things consistent.

@faustbrian
Copy link
Contributor Author

  1. Makes sense.
  2. Makes sense.
  3. Makes sense.
  4. Makes absolutely no sense.
  5. Makes sense.
  6. Makes sense.
  7. No clue where arkjs has been seen since the release of core.

@geopsllc
Copy link

Found a few more ark references, some coming from the develop merge:
import { client as ark }
builder = ark.getBuilder().*
expect(expect("invalid-address").toBeArkAddress).toThrowError("Expected value to be a valid address");
expect(expect("invalid-pubkey").toBeArkPublicKey).toThrowError("Expected value to be a valid public key");

  1. This was just to rebrand the code comments... not a must of course.
  2. You're right ... it's not in the code. It's just in a couple comments:
    packages/core-forger/src/manager.ts: // technically it is possible to compute doing shennanigan with arkjs.slots lib
    packages/core-forger/dist/manager.js: // technically it is possible to compute doing shennanigan with arkjs.slots lib

@geopsllc
Copy link

I tested this branch with my custom installer on devnet and it works perfectly fine. Thanks!

@fix
Copy link
Contributor

fix commented Jan 15, 2019

Great move.

Just one 'philosophical' point: as soon as the sidechain builder has 'named' his sidechain, it should possible to 'call' it with the name.

Specifically for instance :

  • the database name/user
  • the adresses (create an ark address or a dark, phantom, persona address etc...)
  • the nodes/network (connect to an ark/mainnet node)

Also loosely linked to this aspect, desktop/mobile wallet should be able to 'reflect' custom sidechain (logo, colors, ticket etc...) Maybe published on autoconfigure endpoint

@geopsllc
Copy link

Everything is quite generic right now which makes it very flexible to modify for other chains. The database name/user is already set by the installer script so what's in the code doesn't matter - generic 'core' is fine. I can see .core/ and core/ being a problem and I guess it can be reverted to .ark/ and ark-core/ if it is so desired. I'm done with a script already that can change those paths in the code on the fly. The only issue with that approach is updating the core afterwards will fail if any of the files containing the paths are modified in the repo.

P.S. The latest merge didn't pass the checks ... there is one hard-coded ark-core path.

@faustbrian faustbrian merged commit a7e7cb6 into develop Jan 16, 2019
@faustbrian faustbrian deleted the dbrand branch January 16, 2019 12:47
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.

5 participants