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.

Sign up for GitHub

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

Jump to bottom

feat(example): plumbing, intra-node routing for #308 #337

Merged
merged 43 commits into from
Dec 1, 2020
Merged

feat(example): plumbing, intra-node routing for #308 #337

merged 43 commits into from
Dec 1, 2020

Conversation

petermetz
Copy link
Contributor

As per the change request: #308 (comment)

Moreover, I don't feel comfortable because the title of your PR is "docs" and this PR includes not only documents but also actual codes. Would you mind if you divide your PR into docs(example) and feat(example)?

cc: @takeutak

@petermetz petermetz added enhancement New feature or request API_Server dependencies Pull requests that update a dependency file Significant_Change Applying this label triggers the more stringent review of the maintainers and the 2+1 PR rule. labels Nov 2, 2020
@petermetz petermetz added this to the v0.2.0 milestone Nov 2, 2020
@petermetz petermetz mentioned this pull request Nov 2, 2020
Merged
jonathan-m-hamilton
jonathan-m-hamilton approved these changes Nov 3, 2020
View reviewed changes
Copy link
Contributor

@jonathan-m-hamilton jonathan-m-hamilton left a comment

Choose a reason for hiding this comment

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

LGTM

@petermetz
build(webpack): add JSDoc type hints to config
60c62ab 
This enables auto-complete in the IDE while typing
webpack configuration. Other than this it does not
have any effect whatsoever.

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
feat(common): add Servers.startOnPreferredPort(), Servers.startOnPort()
43ab0f6 
Useful convenience method for launching NodeJS
HTTP servers and having a preferred port, but
also
not minding if it has to
be allocated randomly.

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
chore(typings): add async-exit-hook npm package typings
47974cf 
This is necessary because the mentioned
package does not have typings publicly available.

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
build(package-json): configure now runs install and dev build only
b65cbd8 
This is better aligned with what the script is used for in general.

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
build(package-json): npm script to run tests quorum connector only
cde3b4e 
Comes in handy when working on that specific package.

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
fix(cmd-api-server): add IPluginImport to public API surface of package
b929ac7 
Previously this was missing from the package exports
and therefore was not accessible to
other
packages using this one.

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
fix(cmd-api-server): plugin imports via static config (env,cli,file)
15e8c5e 
Depends on webpack/webpack#11316
This has no real effect until the PR above is
merged and releaed on Webpack
(and then we upgrade to that release for the builds)

How this fix works is explained in the linked PR
in greater detail.

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
build(commitizen): limit header to 72 from 100
a8db888 
This is necessary to match the defaults that Husky is enforcing.

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
chore: update @types/express to 4.17.8
01a2a78 
This is consolidates the verion of this
dependency across the different packages.
Also had to resolve some compilation issues
that got introduced by the upgraded version
which now requires that the first parmaeter
of the listen handler be optional (the error).

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
refactor(core-api): add transact() to connector, remove addPublicKey()
d1514bd 
Also:
1. IWebServiceEndpoint#registerExpress(), getVerbLowerCase() to facilitate automated endpoint
registration with reduced boilerplate in the plugin implementations.
2. Added cactus-core package
with a utility method that can register the endpoints into plugins in a generic way leveraging the
new methods described above.

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
build(test-plugin-consortium-manual): fix bundle names in package.json
da2752f 
There was a copy paste mistake that left this package pretending that it was the quorum connector

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
refactor(core-api): renames the plugin id getter to package name
82a9b7e 
Doing so better represents what the getter is actually for.
Calling it plugin ID is semantically
incorrect because
It does not uniquely identify the plugin instance
only the plugin package.

Fixes hyperledger-cacti#262

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
test(common): experiment to fix flaky test case
1c12daf 
Might not work out, but worth a try to refactor it this way:
Moved all the test setup logic to the
test cases
instead of having it dangle in the global context.

Fixes hyperledger-cacti#299 (potentially)

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
feat(common): add OpenAPI spec exporter utility function
477e6b4 
This will be useful to reuse among the individual packages
own openapi-spec.ts files which at the
moment
all repeat the same logic to export themselves to JSON...

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
build(tsconfig-json): include es2019 in libs to have Array#flat()
f2f23ef 
With this we no longer need to include lodash for flattening 2D arrays
which is great when
optimizing for bundle sizes in the browser builds.

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
fix(common): servers.startOnPort() error handling
d7bcaf9 
Promise no longer hangs if error event is fired on the server object

Fixes hyperledger-cacti#317

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
feat(common): add IAsyncProvider interface definition
968f3e8 
This is a commonly reusable interface that objects/classes
can choose to implement to express that they have
the ability to provide a certain value.
Helps a lot while authoring methods that must be able
to accept injected providers as parameters for example
in order to allow them to be customizable without
doing an actual source code rewrite of the
functions itself.

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
refactor: move consortium OpenAPI schema to core-api
ec6c577 
Also: add the Ledger and LedgerType schemas
Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
feat: 🎸 add method: QuorumTestLedger#getGenesisAccount()
b335363 
A utility method on to easily obtain an account from the genesis
allocation that has a high balance to seed other accounts without having
to mine for them (which doesn't make sense in a quorom ledger for us in
a testing environment)

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
feat(besu): 🎸 OpenAPI support => arbitrary TX, contract deployments
08a77c2 
This brings the besu connector to feature parity with the quorum
connector which already had these features supported (the code is mostly
taken from the Quorum connector because both Besu and Quorum connectors
are using Web3 under the hood so they share most of the logic/OpenAPI
definitions, etc.\nDifferences between the two will come out once we
start implementing private transactions and the other features that are
unique to the ledgers are not present/supported by vanilla
Web3.\nPerhaps it would be good to somehow have a third package that
allows for both Besu and Quorum connectors to re-use the vanilla Web3
specific parts instead of copy pasting, but this is a shortcut that was
deemed acceptable for now as we are ramping up for the v0.2.0 release.

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
chore(besu): 🤖 migrate from tap to tape
a408c62 
Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
test(common): 💍 Bools.isBooleanStrict()
116b87b 
Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
feat(core-api): 🎸 add IKeychainPlugin#getKeychainId()
f5fe75d 
New method on the generic interface for keychain plugins that enables
the handling of multiple different keychain backends on the same API
server.

This is useful if you want to have different instances of keychain
plugins deployed because for example your consortium member operates
multiple keychain backends such as one provided by the cloud provider
of their choice and another one that could be a self-hosted, open
source software deployment.
Cactus aims not to limit the deployment architecture where possible
and this feature is aimed at maintaining that design principle.

The idea is that API requests can specify which keychain they want
to use when looking up the signing key for a transaction.
The implementation that serves the request then reaches to the
PluginRegistry and retrieves a list of keychain plugins, then
proceeds to filter that list down to just the one instance based
on the keychain ID that has to match up to what was specified in
the API request being served.

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
build: 🏗 add top level tape-promise dependency for async tests
6f4fe61 
This can be used for asserting the rejection of certain method calls
while authoring tests. This is useful because you can do it in a
one-liner instead of having to write a try catch block and then manually
assert which branch the code execution took (e.g. did it finish the try
block or did it throw and ran the catch block...)

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
refactor: 💡 In-memory keychain getEncryptionAlgorithm() => null
c8b9df1 
Previously this method would return a string expressing in English prose
that it does not support encryption and therefore there are no
algorithms there either. It is deemed safer to return null because that
way no one could construe that there is a valid value in there (which
could indeed happen with the previous version if one was just checking
whether a truthy value has been returned by the method or not).

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
test: 💍 add plugin-keychain-memory unit test
592e81e 
Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
feat(quorum): 🎸 export model-type-guards to public API surface
911cf89 
These are handy utility functions that can narrow the types of
Web3SigningCredentialType instances.

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
feat(quorum): 🎸 support Cactus Keychain APIs
04e354a 
1. The OpenAPI spec of the quorum connector has been updated to include
a new web3 signing credential type pertaining to a case where the caller
passes in a reference to a keychain entry and the ID of the keychain
where the entry can be located.

2. The quorum connector can now
execute transactions with the credential type mentioned above so that
passwords/private keys do not have to travel over the wire unless that
is the explicit intention of the caller (since it is still a supported
option)

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
test(quorum): 💍 ensure keychain refs work
6981f48 
This test verifies that the Quorum connector plugin is indeed capable of
accepting transaction requests that do not contain explicitly the
signing credentials but instead provide a reference to a keychain entry
accessible through the keychain plugin.

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
feat(besu): add keychain reference signing support
95f4dbd 
Also
1. increases test coverage
2. updates the test contract to have state altering methods

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
feat(tools): besu-all-in-one personal RPC HTTP API
6f8c5e3 
This is required for server-side (as in, geth) tx signing
from the geth keychain.

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
feat(test-tooling): 🎸 createEthTestAccount() to Besu,Quorum test ledger
7a854fe 
Adds utility methods to the test ledger classes that can create an ETH
account from scratch and send it some initial money to get going with.
Added the same functionality for both Quorum and Besu at the same time
because they were very similar to begin with.

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
build: 🏗 configure script runs full dev build
ebbe064 
Previously it would only run the backend dev build (e.g. omitting the
frontend packages) which was okay until we started introducing the
examples packages which only work if you also build the frontend.
Therefore this change is necessary to make sure we can keep the Getting
Started section of the README.md file short and sweet by having it say
to run the configure script and then launch the example app.

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
build(tap): run tests with 4 GB JS heap
c67c544 
Necessary because with the addition of the
cactus-example-* packages the footprint
of the code has grown to the point where
2 GB default heap size is not cutting it
anymore apparently.
See example of a crash due to lack of
available memory here:
https://travis-ci.org/github/hyperledger/cactus/jobs/739153212#L10922

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
fix(common): flaky KeyConverter unit tests
51ef027 
Fixes hyperledger-cacti#299

This does not provide a real fix for the
issue just marks the tests to be skipped
for now until we complete the migration
to a different test runner which will
implicitly provide the real fix as per
hyperledger-cacti#299 (comment) =>

Finally had the time to investigate this
properly and it is an issue that traces
back to the tap test executor unfortunately.
We already had plans to replace tap with
tape for other reasons (adding browser
testing support) but now this has
become more important than ever.

While seeing if we could use tape as the
test runner as well, I arrived at the
conclusion that we cannot because it
does not support obtaining coverage
while also compiling the Typescript code on the fly.
Another executor that does support this
and also satisfies our requirement of
being able to spit out TAP formatted
results is AVA so I will update the
relevant GH issue hyperledger-cacti#238 about test
runner migration to target AVA instead of tape...

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@takeutak
Copy link
Contributor

takeutak commented Nov 25, 2020 •
edited
Loading

@petermetz Sorry for late message. What are the codes reviewed on this pull-request? Are they all 166 codes on "Files changed" or the part filtered with some commit name?

takeutak
takeutak reviewed Nov 27, 2020
View reviewed changes
Copy link
Contributor

@takeutak takeutak left a comment

Choose a reason for hiding this comment

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

@petermetz I almost agree to these pull-request. But I'm wondering the config of emojis on changelog.config.js. Would you mind if you delete these config of emojis?

changelog.config.js Outdated Show resolved Hide resolved
@takeutak
Copy link
Contributor

@petermetz Moreover, as above, I think there will be no problem except the changelog setting. However, from the next time, when issuing multiple Pull requests, please do not create Pull requests that depend heavily on other Pull requests, such as by separating branches. To be honest, it is very difficult to review.

@petermetz
style: remove confusing emojis from changelog.config.js
30ad1b4 
✅ Closes: hyperledger-cacti#337 (comment)

Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
Copy link
Contributor Author

@petermetz I almost agree to these pull-request. But I'm wondering the config of emojis on changelog.config.js. Would you mind if you delete these config of emojis?

@takeutak Done: 30ad1b4

@petermetz
Copy link
Contributor Author

However, from the next time, when issuing multiple Pull requests, please do not create Pull requests that depend heavily on other Pull requests, such as by separating branches.

@takeutak If we fix the root cause, then I can stop submitting dependent PRs, yes, but until then this is the only solution I could come up with so far. (always open to alternative solutions of course)
Root cause: The average PR review time is 2 to 3 weeks even for PRs where I only change a few lines of code. Without the dependent PRs, I would be forced to spend 80% of my time idling or just spend the same time rebasing+conflict resolving dozens of branches daily.

To be honest, it is very difficult to review.

@takeutak I hear you. Could you please point out which step is the difficult one from the step by step guides I provided in the past? I'm happy to expend on said steps, go deeper into specific parts of it if you give me some pointers.

Also happy to do pair programming style reviews where we do screen sharing and I assist with performing the reviews via the either the GitHub UI (or other preferred tool such as VSCode)

@takeutak takeutak self-requested a review December 1, 2020 02:58
@takeutak
Copy link
Contributor

takeutak commented Dec 1, 2020

LGTM

takeutak
takeutak approved these changes Dec 1, 2020
View reviewed changes
Copy link
Contributor

@takeutak takeutak left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM.
@petermetz Please make the same correction for other your dependent pull requests before they are merged. Otherwise, when merging dependent pull-requests, the modifications will be reversed by mistake.

@petermetz
Copy link
Contributor Author

LGTM.

@takeutak Cheers!

@petermetz Please make the same correction for other your dependent pull requests before they are merged. Otherwise, when merging dependent pull-requests, the modifications will be reversed by mistake.

@takeutak Yeah, thankfully, rebase takes care of that automatically (assuming no conflicts) It's a big part of the reason why I strongly prefer rebase to merges in general.

@petermetz petermetz merged commit e76b55c into hyperledger-cacti:master Dec 1, 2020
@petermetz petermetz deleted the feat/example branch December 1, 2020 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonathan-m-hamilton jonathan-m-hamilton jonathan-m-hamilton approved these changes

@takeutak takeutak takeutak approved these changes

@sfuji822 sfuji822 Awaiting requested review from sfuji822

Assignees
No one assigned
Labels
API_Server dependencies Pull requests that update a dependency file enhancement New feature or request Significant_Change Applying this label triggers the more stringent review of the maintainers and the 2+1 PR rule.
Projects
None yet
Milestone
v0.2.0
Development

Successfully merging this pull request may close these issues.
3 participants
@petermetz @takeutak @jonathan-m-hamilton