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: BusinessRegistration Transaction Implementation (AIP-103) #2791

Closed
wants to merge 19 commits into from
Closed

Conversation

KovacZan
Copy link
Contributor

@KovacZan KovacZan commented Jul 10, 2019

This pull request implements Business transaction type from AIP 103

Would like to get some feedback on the current state of the code, so i can adjust it properly

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Refactor
  • Performance
  • Tests
  • Build
  • Documentation
  • Code style update
  • Continuous Integration
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes
  • No

Does this PR release a new version?

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

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

@ghost
Copy link

ghost commented Jul 10, 2019

The ci/circleci: node11-unit job is failing as of cbb65263238ceeab4604a10cbb68e97356f67a26. Please review the logs for more information.

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

@ghost
Copy link

ghost commented Jul 10, 2019

The ci/circleci: node11-integration job is failing as of cbb65263238ceeab4604a10cbb68e97356f67a26. Please review the logs for more information.

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

@KovacZan KovacZan marked this pull request as ready for review July 12, 2019 08:59
Copy link
Contributor

@air1one air1one left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a first look, looks good, good job ! See my comment for hasByBusinessName and similar methods.

Also, can you try to add functional tests inspired by the existing ones ? To make sure that in a "real" environment the transactions are accepted and forged.

Thanks !

@ghost
Copy link

ghost commented Jul 12, 2019

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

@codecov-io
Copy link

codecov-io commented Jul 16, 2019

Codecov Report

Merging #2791 into 2.6 will decrease coverage by 0.19%.
The diff coverage is 66.15%.

Impacted file tree graph

@@            Coverage Diff            @@
##              2.6    #2791     +/-   ##
=========================================
- Coverage   64.99%   64.79%   -0.2%     
=========================================
  Files         366      385     +19     
  Lines        8367     8632    +265     
  Branches      440      414     -26     
=========================================
+ Hits         5438     5593    +155     
- Misses       2891     3002    +111     
+ Partials       38       37      -1
Impacted Files Coverage Δ
packages/core/src/commands/network/generate.ts 0% <ø> (ø) ⬆️
packages/crypto/src/transactions/types/schemas.ts 100% <100%> (ø) ⬆️
...ons/builders/transactions/business-registration.ts 100% <100%> (ø)
...to/src/transactions/types/business-registration.ts 100% <100%> (ø)
...core-transactions/src/handlers/handler-registry.ts 93.47% <100%> (+0.29%) ⬆️
packages/crypto/src/transactions/registry.ts 57.14% <100%> (+1.04%) ⬆️
packages/core-state/src/wallets/wallet.ts 87.5% <100%> (+0.15%) ⬆️
packages/crypto/src/enums.ts 100% <100%> (ø) ⬆️
...transactions/src/handlers/business-registration.ts 23.4% <23.4%> (ø)
packages/core-state/src/wallets/wallet-manager.ts 74.64% <50%> (-2.7%) ⬇️
... and 23 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 b14fe92...0d56f48. Read the comment docs.

@KovacZan KovacZan closed this Jul 18, 2019
@ghost
Copy link

ghost commented Jul 18, 2019

Your pull request has been closed, thank you for trying to solve an issue. If you think it was closed prematurely please provide additional information.

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