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: only connect to ledger when needed #273

Merged

Conversation

TimoGlastra
Copy link
Contributor

@TimoGlastra TimoGlastra commented May 14, 2021

  • only connect to ledger when needed
  • allow to pass genesisTransactions so you don't have to deal with storing the genesis yourself

Fixes #219
Fixes #265

The ledger management can be improved, but I think this is a simple first step. Only open the ledger connection if a ledger call is made


Tested in both NodeJS and React Native

@TimoGlastra TimoGlastra requested a review from a team as a code owner May 14, 2021 21:38
@codecov-commenter
Copy link

codecov-commenter commented May 14, 2021

Codecov Report

Merging #273 (52c2bd4) into main (21b15ff) will increase coverage by 0.00%.
The diff coverage is 81.57%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #273   +/-   ##
=======================================
  Coverage   88.75%   88.76%           
=======================================
  Files         215      216    +1     
  Lines        3878     3897   +19     
  Branches      436      438    +2     
=======================================
+ Hits         3442     3459   +17     
- Misses        436      438    +2     
Impacted Files Coverage Δ
src/storage/fs/ReactNativeFileSystem.ts 0.00% <0.00%> (ø)
src/types.ts 100.00% <ø> (ø)
src/modules/indy/services/IndyIssuerService.ts 67.64% <50.00%> (+0.98%) ⬆️
src/utils/path.ts 50.00% <50.00%> (ø)
src/modules/ledger/services/LedgerService.ts 84.84% <91.30%> (+0.81%) ⬆️
src/agent/Agent.ts 98.59% <100.00%> (-0.04%) ⬇️
src/agent/AgentConfig.ts 95.74% <100.00%> (+0.18%) ⬆️
src/modules/ledger/LedgerModule.ts 84.61% <100.00%> (-1.10%) ⬇️
src/storage/fs/NodeFileSystem.ts 58.82% <100.00%> (+23.10%) ⬆️

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 21b15ff...52c2bd4. Read the comment docs.

@TimoGlastra TimoGlastra force-pushed the ledger-improvements branch from 2803bfb to 5ff017c Compare May 16, 2021 14:10
@TimoGlastra TimoGlastra force-pushed the ledger-improvements branch from 5ff017c to 52c2bd4 Compare May 17, 2021 07:06
@TimoGlastra TimoGlastra enabled auto-merge (squash) May 17, 2021 07:06
@jakubkoci
Copy link
Contributor

There is this mention "if not open it and then close it after not being used for e.g. two minutes" in #265. Is it also implemented in this PR? Are we able to somehow detect that the connection to the ledger has been lost?

@@ -16,10 +16,6 @@ export class LedgerModule {
this.wallet = wallet
}

public async connect(poolName: string, poolConfig: LedgerConnectOptions) {
return this.ledgerService.connect(poolName, poolConfig)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this one also public?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops forgot about that one

@TimoGlastra TimoGlastra merged commit a9c261e into openwallet-foundation:main May 17, 2021
Copy link
Contributor

@jakubkoci jakubkoci left a comment

Choose a reason for hiding this comment

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

We should just make the connect method in the module public. My other comment is not a blocker for this PR.

@TimoGlastra
Copy link
Contributor Author

There is this mention "if not open it and then close it after not being used for e.g. two minutes" in #265. Is it also implemented in this PR? Are we able to somehow detect that the connection to the ledger has been lost?

No that's not implemented as part of this PR, I wanted to address that in a separate PR.

Next step is allowing to close the pool and reconnect. But to keep PRs smaller I'm taking baby steps :)

@jakubkoci
Copy link
Contributor

Next step is allowing to close the pool and reconnect. But to keep PRs smaller I'm taking baby steps :)

That's good 👍 of course.. I just wanted to mention it because the issue is closed by this PR now, but it could be good to have some opened for this work ;)

@TimoGlastra
Copy link
Contributor Author

I created a new issue #277.

Could you make the CI jobs required for merge? That's easier with auto-merge. I don't have the permission to do so

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.

Improve ledger pool management Pass a Genesis string to agent config instead of a file path
3 participants