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(jellyfish-api-core): add network getPeerInfo RPC #938

Merged
merged 11 commits into from
Jan 17, 2022

Conversation

kodemon
Copy link
Contributor

@kodemon kodemon commented Jan 4, 2022

What this PR does / why we need it:

/kind feature

Which issue(s) does this PR fixes?:

Added getPeerInfo rpc as part of ongoing #48

@kodemon kodemon added the kind/feature New feature request label Jan 4, 2022
@kodemon kodemon self-assigned this Jan 4, 2022
@defichain-bot defichain-bot added area/packages and removed kind/feature New feature request labels Jan 4, 2022
@codeclimate
Copy link

codeclimate bot commented Jan 4, 2022

Code Climate has analyzed commit 2649d77 and detected 0 issues on this pull request.

View more on Code Climate.

@codecov
Copy link

codecov bot commented Jan 4, 2022

Codecov Report

Merging #938 (2649d77) into main (c237ca0) will increase coverage by 0.55%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #938      +/-   ##
==========================================
+ Coverage   94.52%   95.07%   +0.55%     
==========================================
  Files         155      158       +3     
  Lines        4763     4834      +71     
  Branches      613      634      +21     
==========================================
+ Hits         4502     4596      +94     
+ Misses        261      230      -31     
- Partials        0        8       +8     
Impacted Files Coverage Δ
packages/jellyfish-api-core/src/category/net.ts 100.00% <100.00%> (ø)
...s/testcontainers/src/containers/DockerContainer.ts 98.21% <100.00%> (+5.19%) ⬆️
...ckages/ocean-api-client/src/errors/ApiException.ts 47.22% <0.00%> (-52.78%) ⬇️
...an-api-client/src/errors/ApiValidationException.ts 50.00% <0.00%> (-50.00%) ⬇️
packages/jellyfish-api-core/src/category/spv.ts 72.09% <0.00%> (-13.96%) ⬇️
...ellyfish-transaction/src/script/dftx/dftx_token.ts 88.37% <0.00%> (-11.63%) ⬇️
.../src/containers/RegTestContainer/ContainerGroup.ts 87.23% <0.00%> (-8.52%) ⬇️
...transaction-builder/src/txn/txn_builder_account.ts 94.28% <0.00%> (-5.72%) ⬇️
...ners/src/containers/RegTestContainer/Masternode.ts 91.58% <0.00%> (-5.61%) ⬇️
...ners/src/containers/RegTestContainer/Persistent.ts 92.59% <0.00%> (-3.57%) ⬇️
... and 31 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 c237ca0...2649d77. Read the comment docs.

@fuxingloh
Copy link
Contributor

We also moved to "Semantic" style PR title instead of Chat Ops via /kind feature.

This means your PR title should be feat(jellyfish-api-core): add network getPeerInfo RPC

@kodemon kodemon changed the title Add getPeerInfo rcp feat(jellyfish-api-core): add network getPeerInfo RPC Jan 4, 2022
@defichain-bot defichain-bot added the kind/feature New feature request label Jan 4, 2022
 - Account for undefined values
 - Perform key=>value pair checking in 1 schema test instead of 2 (is sort of redundant).
@netlify
Copy link

netlify bot commented Jan 5, 2022

✔️ Deploy Preview for jellyfish-defi ready!

🔨 Explore the source changes: e95cbd0

🔍 Inspect the deploy log: https://app.netlify.com/sites/jellyfish-defi/deploys/61e504f92175ae00073380b2

😎 Browse the preview: https://deploy-preview-938--jellyfish-defi.netlify.app

@kodemon kodemon requested a review from canonbrother January 5, 2022 03:20
Matching the other tests in the net package.
Forgot to loop through the newly retrieved list of peers avoiding a second call for peers in the same test.
@kodemon
Copy link
Contributor Author

kodemon commented Jan 6, 2022

Currently experiencing some difficulty properly getting stable unit tests passing.

During testing I am failing to consistently being able to retrieve a list of peers from the test container.

While digging through the various test container I found that an approach @chee-chyuan used performs an .addNode surrounded by some network requirements. Which differs from the approach I am taking in this PR. I am not sure if I should be trying to use the group container although I do not need the group functionality, or if there is another way to ensure a consistent peerInfo list result.

Here is some links to logic on group container:

https://github.com/DeFiCh/jellyfish/blob/main/packages/testcontainers/src/containers/RegTestContainer/ContainerGroup.ts#L43-L46

Here its starting the containers and running the .add method which triggers:

https://github.com/DeFiCh/jellyfish/blob/main/packages/testcontainers/src/containers/RegTestContainer/ContainerGroup.ts#L71-L79

Here I am noting line 72 which performs a require network function and then performing an .addNode.

In this PR I am using the .addNode as noted here: https://github.com/DeFiCh/jellyfish/pull/938/files#diff-9a976903bc2708f896fec50b1072d524efe1f494c867727b367a61e029a5b051R14 but this does not come with the require network part. I am not sure if this is needed or not.

After digging through the various test containers I didn't see a clean way or reason for why this inconsistency occurs so will need an assist at troubleshooting this specific case.

Copy link
Contributor

@canonbrother canonbrother left a comment

Choose a reason for hiding this comment

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

general wise.. good for me

ohh.. seems some issue here

@kodemon kodemon requested a review from canonbrother January 6, 2022 07:32
chee-chyuan
chee-chyuan previously approved these changes Jan 6, 2022
surangap
surangap previously approved these changes Jan 7, 2022
Copy link
Contributor

@fuxingloh fuxingloh left a comment

Choose a reason for hiding this comment

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

You should optimize your test for ease of review rather than ease of implementation.

@kodemon kodemon dismissed stale reviews from surangap and chee-chyuan via e95cbd0 January 17, 2022 05:56
@canonbrother
Copy link
Contributor

canonbrother commented Jan 17, 2022

@kodemon
my bad as missing the waitForSync
can you help add

await tGroup.waitForSync()

at packages/jellyfish-api-core/__tests__/category/loan/takeLoan.test.ts:347
?? 😅

https://github.com/DeFiCh/jellyfish/blob/e95cbd09c2a5d4dc112e68c29a9aff5289a4e574/packages/jellyfish-api-core/__tests__/category/loan/takeLoan.test.ts#L341-L343

@kodemon
Copy link
Contributor Author

kodemon commented Jan 17, 2022

@canonbrother am I correct in assuming we want to perform a wait for sync operation after each generate call, or is specifically only at this particular line?

@canonbrother
Copy link
Contributor

canonbrother commented Jan 17, 2022

@canonbrother am I correct in assuming we want to perform a wait for sync operation after each generate call?

like this
eg:

alice.rpc.call...
waitForSync
// do waitForSync while getting another node call
bob.rpc.call

after sync bob will have same data with alice

@kodemon
Copy link
Contributor Author

kodemon commented Jan 17, 2022

after sync bob will have same data with alice

Thanks, makes sense, in this case bob creates a vault, and we need to wait for all containers to sync so that alice can perform a deposit to the vault that bob created.

@fuxingloh fuxingloh merged commit d74e806 into main Jan 17, 2022
@fuxingloh fuxingloh deleted the kodemon/rpc-getpeerinfo branch January 17, 2022 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants