Skip to content

refactor(configuration): Support Bridgechains & Custom Configurations#105

Merged
faustbrian merged 4 commits intoArkEcosystemArchive:developfrom
sleepdefic1t:refactor(configuration)/support-bridgechains
Jul 2, 2019
Merged

refactor(configuration): Support Bridgechains & Custom Configurations#105
faustbrian merged 4 commits intoArkEcosystemArchive:developfrom
sleepdefic1t:refactor(configuration)/support-bridgechains

Conversation

@sleepdefic1t
Copy link
Contributor

@sleepdefic1t sleepdefic1t commented Jul 1, 2019 •
edited
Loading

The current implementation only allows use with Devnet unless values are manually changed in /src.
This was never fully intended, as such this is submitted as a refactor as opposed to a feature PR.

These changes enable network configuration via the ARK Crypto library api.
(Devnet, Mainnet, Testnet, and Custom Networks (bridgechains).

Specifically this PR does the following:

  • Creates a Configuration class.
  • Creates Fee and Network Manager bases for the Configuration class.
  • Allows passing a Configuration to Transaction Builder with a default value of Devnet & StaticFees (non-breaking).
  • Adds a FeePolicy-type.
  • Adds a Fee class (container for fee policies).
  • Reimplements Static Fees as a FeePolicy-type (breaking).
  • Adds a Networks class (base for preset networks).
  • Updates the TransactionTypes enum.
  • Reimplements and improves the Network and Networks classes (breaking).
  • Updates the Slot class to reflect changes (non-breaking).
  • Updates Transaction-related classes to reflect updates (non-breaking).
  • Adds tests for all changes.
  • Updates the Arduino IDE script.
  • Updates .ino Arduino sketches with Configuration examples.
  • Updates documentation with Configuration examples.
  • Updates the keywords.txt file to reflect changes.
  • Updates the changelog.
  • Updates version to v.0.6.0.

What kind of change does this PR introduce?

  • Refactor

Does this PR introduce a breaking change?

  • Yes
  • No

Does this PR release a new version?

  • Yes
  • No

There are two breaking changes in this PR.
(e.g. Devnet vs Devnet(), and Fees vs Fees::StaticFeePolicy)

The PR fulfills these requirements:

  • It's submitted to the develop branch, not the master branch
  • All tests are passing
  • New/updated tests are included

@sleepdefic1t
refactor(configuration): support bridgechain configurations
6feae41 
The current implementation only allows use with Devnet unless values are manually changed in `/src`.

This PR enables network configuration via the public api.
(Devnet, Mainnet, Testnet, and Custom Networks (bridgechains).

Specifically this PR does the following:
- Creates a Configuration class.
- Creates Fee and Network Manager children for the Configuration class.
- Allows passing a `Configuration to` Transaction Builder with a default value of `Devnet` & `StaticFees` (non-breaking).
- Adds a FeePolicy-type.
- Adds a Fee class (container for fee policies).
- Adds a Networks class (container for preset networks).
- Updates the TransactionTypes enum.
- Improves the `Network` class and Network implementions.
- Updates the `Slot` class to reflect changes (non-breaking).
- Updates Transaction-related classes to reflect updates (non-breaking).
- Adds tests for all changes.
- Updates the Arduino IDE script.
- Updates `.ino` Arduino sketches with Configuration examples.
- Updates documentation with Configuration examples.
- Updates the `keywords.txt` file to reflect changes.
- Updates the changelog.
- Updates version to `v.0.6.0`.

Some small changes are technically breaking-changes.
(e.g. `Devnet` vs `Devnet()`, and `Fees` vs `Fees::StaticFeePolicy`)
@ghost ghost added Complexity: Undetermined Needs specialized, in-depth review. Type: Refactor The pull request improves or enhances an existing implementation. labels Jul 1, 2019
@codecov-io
Copy link

codecov-io commented Jul 1, 2019 •
edited
Loading

Codecov Report

Merging #105 into develop will increase coverage by 0.37%.
The diff coverage is 86.4%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #105      +/-   ##
===========================================
+ Coverage    91.14%   91.52%   +0.37%     
===========================================
  Files           27       30       +3     
  Lines          836      861      +25     
===========================================
+ Hits           762      788      +26     
+ Misses          74       73       -1
Impacted Files Coverage Δ
src/transactions/transaction.cpp 88.94% <100%> (ø) ⬆️
src/include/cpp-crypto/common/configuration.hpp 100% <100%> (ø)
src/common/network.cpp 100% <100%> (ø)
src/utils/slot.cpp 100% <100%> (ø) ⬆️
src/networks/testnet.cpp 100% <100%> (ø)
src/defaults/static_fees.cpp 100% <100%> (ø)
src/managers/network_manager.cpp 100% <100%> (ø)
src/networks/devnet.cpp 100% <100%> (ø)
src/networks/mainnet.cpp 100% <100%> (ø)
src/managers/fee_manager.hpp 100% <100%> (ø)
... and 19 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 ed0ab57...b1a8ac5. Read the comment docs.

@faustbrian
Copy link
Contributor

faustbrian commented Jul 1, 2019

I will take a look when I am back in a few days but just a quick note on the public API part (disregard if not relevant)

All our crypto packages are being built with the lack of any knowledge about networking as that isn't and shouldn't be of any concern for crypto to work. How the developer decides to feed the configuration to the package is also not the concern of the crypto package, the only concern it has is to receive the configuration in a specific format.

If that configuration came from a remote API, file or function that just generated an object isn't our concern and we shouldn't be involved in that process.

@sleepdefic1t sleepdefic1t mentioned this pull request Jul 1, 2019
Merged
6 tasks
@sleepdefic1t
Copy link
Contributor Author

sleepdefic1t commented Jul 1, 2019 •
edited
Loading

@faustbrian
No worries, I should clarify.
The public API I was referring to is the ARK Crypto library API, not the ARK Client API.

@faustbrian
Copy link
Contributor

faustbrian commented Jul 1, 2019

All good then, get paranoid when people submit PRs with the words public API to the crypto packages :trollface:

Will be back normal around thursday/friday and then take a look at the whole PR.

@ghost
Copy link

ghost commented Jul 1, 2019

The ci/circleci: build-macos-9-2 job is failing as of 2510281468e9ea2a7bbf41ae56d5611520d65c65. Please review the logs for more information.

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

1 similar comment
@ghost
Copy link

ghost commented Jul 1, 2019

The ci/circleci: build-macos-9-2 job is failing as of 2510281468e9ea2a7bbf41ae56d5611520d65c65. Please review the logs for more information.

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

faustbrian added a commit that referenced this pull request Jul 2, 2019
@faustbrian @ciband
chore(arduino): merge changes at #105 for Arduino. (#106)
1a4eac3 
chore(arduino): merge changes at #105 for Arduino.

Co-authored-by: Chris Johnson <chrisjohnsonmail@gmail.com>
Co-authored-by: Brian Faust <faustbrian@users.noreply.github.com>
@faustbrian faustbrian merged commit 00575f7 into ArkEcosystemArchive:develop Jul 2, 2019
@sleepdefic1t sleepdefic1t deleted the refactor(configuration)/support-bridgechains branch July 3, 2019 01:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kristjank kristjank Awaiting requested review from kristjank

@spkjp spkjp Awaiting requested review from spkjp

1 more reviewer

@faustbrian faustbrian faustbrian approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Complexity: Undetermined Needs specialized, in-depth review. Type: Refactor The pull request improves or enhances an existing implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sleepdefic1t @codecov-io @faustbrian