feat(core): support multiple indy ledgers #474

TimoGlastra · 2021-09-26T21:29:01Z

Adds support for using multiple indy ledgers simultaneously. I'd really like some input on the approach before wrapping up and merging.

Only supports multiple ledgers for reading, writing to the ledger still only supports one pool, which is currently always first configured pool.
genesisPath, poolName and genesisTransactions in the InitConfig are replaced by the indyLedgers array property with the following structure: (@MosCD3)

interface IndyPoolConfig {
  genesisPath?: string
  genesisTransactions?: string
  id: string
  isProduction: boolean
}

caching is not implemented yet, I'll do that after validating the approach/implementation
Algorithm to decide which ledger to use is based on this google doc by @swcurran: https://docs.google.com/document/d/109C_eMsuZnTnYe2OAd02jAts1vC4axwEKIq7_4dnNVA/edit#heading=h.564c8ed7spl8

Before each read operation in IndyLedgerService a new method is called, IndyPoolService.getPoolForDid. The method will, if successful, return the pool that should be used for the operation. The process is a follows:

If no pools are configured, an error will be thrown immediately
loop through all configured pools and performs a getNymRequest.
1. If one of the responses is not successful, and is other than LedgerNotFound, an error will be thrown immediately. This means if one requests fails with an unknown error, the process fails. This is to prevent an incorrect pool from being returned.
2. If all pools couldn't find the did, a LedgerNotFoundError will be thrown
A divide between production and non production ledgers is made.
1. If there is one or more successful responses from production ledgers, this is assigned as the remaining group
2. If there are no successful responses from production ledgers, and there are one or more successful responses from non production ledgers, the non production results are assigned as the remaining group.
If there is only one response in the remaining group, return the pool associated with the response
Loop through all remaining responses and check for each if the did is self certifying.
1. If exactly one of the DIDs is self certifying, return the pool associated with it.
If there's still no pool selected, sort all items in the remaning group based on txnTime. The pool associated with the earliest txnTime will be returned.

@swcurran @shaangill025 I'm not so sure on 6. in the process. From the google document I understand that the txnTime is not the same as the time the DID was registered. In the HackMD document from @shaangill025 (https://hackmd.io/BVye073CTiqgNEMXxDeVdQ) the following step is described: "return production ledger with oldest/min datetime". Where does the oldest/min datetime property come from? I'd rather not have a service that tracks DIDs that are on multiple ledgers, and would be inclined to just throw an error instead that the DID couldn't be resolved.

Signed-off-by: Timo Glastra <timo@animo.id>

packages/core/src/modules/ledger/services/IndyPoolService.ts

packages/core/src/modules/ledger/__tests__/IndyPoolService.test.ts

codecov-commenter · 2021-09-26T21:39:16Z

Codecov Report

Merging #474 (558adfa) into main (af39ad5) will increase coverage by 0.60%.
The diff coverage is 94.70%.

@@            Coverage Diff             @@
##             main     #474      +/-   ##
==========================================
+ Coverage   85.79%   86.39%   +0.60%     
==========================================
  Files         254      266      +12     
  Lines        5525     5726     +201     
  Branches      892      920      +28     
==========================================
+ Hits         4740     4947     +207     
+ Misses        784      778       -6     
  Partials        1        1

Impacted Files	Coverage Δ
packages/core/src/types.ts	`100.00% <ø> (ø)`
packages/core/src/modules/ledger/IndyPool.ts	`84.05% <84.05%> (ø)`
...e/src/modules/ledger/services/IndyLedgerService.ts	`85.71% <90.90%> (+6.19%)`	⬆️
packages/core/src/agent/AgentConfig.ts	`94.33% <100.00%> (+3.11%)`	⬆️
packages/core/src/cache/CacheRecord.ts	`100.00% <100.00%> (ø)`
packages/core/src/cache/CacheRepository.ts	`100.00% <100.00%> (ø)`
packages/core/src/cache/PersistedLruCache.ts	`100.00% <100.00%> (ø)`
packages/core/src/cache/index.ts	`100.00% <100.00%> (ø)`
...kages/core/src/modules/ledger/error/LedgerError.ts	`100.00% <100.00%> (ø)`
...c/modules/ledger/error/LedgerNotConfiguredError.ts	`100.00% <100.00%> (ø)`
... and 9 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 af39ad5...558adfa. Read the comment docs.

MosCD3 · 2021-09-27T06:38:09Z

@TimoGlastra I like the approach since multiple ledger pools are mainly needed mostly in POC and development phases.

berendsliedrecht

I just have some minor points, but overall it seems good to me. Is multi-ledger-support added in any other framework as described in the document?

Also, this might be worth mentioning in the feature list, it is a very nice feature.

packages/core/src/modules/ledger/IndyPool.ts

packages/core/src/modules/ledger/ledgerUtil.ts

packages/core/src/modules/ledger/services/IndyPoolService.ts

TimoGlastra · 2021-09-30T14:43:19Z

Things to add:

Initially do not throw if one of the ledgers errors out.
option to throw error when one of the pools throws a ledger
caching

…upport

Signed-off-by: Timo Glastra <timo@animo.id>

JamesKEbert

This looks great @TimoGlastra! I left minor requests.. and I think there may have been some tweaks still required from the last AFJ call?

docs/getting-started/0-agent.md

packages/core/src/modules/ledger/__tests__/IndyPoolService.test.ts

JamesKEbert · 2021-10-08T19:30:35Z

packages/core/src/modules/ledger/services/IndyPoolService.ts

+import { LedgerError } from '../error/LedgerError'
+import { LedgerNotConfiguredError } from '../error/LedgerNotConfiguredError'
+import { LedgerNotFoundError } from '../error/LedgerNotFoundError'


Could we place all the errors within one file? It seems like it's a little more overhead/verbose to have them separate for less value from my POV?

Sure. I'll do it in a follow up PR to keep consistency.In the rest of the codebase all errors have their own file

Awesome 🔥

…upport

Signed-off-by: Timo Glastra <timo@animo.id>

TimoGlastra · 2021-10-09T13:05:27Z

Resolved feedback from AFJ call and PR.

Ready for final review

JamesKEbert

Again, great work here, I appreciate the efforts put into this!

JamesKEbert · 2021-10-10T00:22:34Z

docs/getting-started/0-agent.md

+- `indyLedgers`: The indy ledgers to connect to. This is an array of objects with the following properties. Either `genesisPath` or `genesisTransactions` must be set, but not both.
+  - `id`\*: The id (or name) of the ledger, also used as the pool name
+  - `isProduction`\*: Whether the ledger is a production ledger. This is used by the pool selector algorithm to know which ledger to use for certain interactions (i.e. prefer production ledgers over non-production ledgers)
+  - `genesisPath`: The path to the genesis file to use for connecting to an Indy ledger.
+  - `genesisTransactions`: String of genesis transactions to use for connecting to an Indy ledger.


Potentially something important to highlight here would be that the order of the ledgers in the array determines their priority (after production vs non-production)? As indicated in a comment in the IndyPoolService:

We take the first value as we take the order in the indyLedgers config as
the order of preference of ledgers

Yes great point. I'll update it

Added some documentation around ledgers

Signed-off-by: Timo Glastra <timo@animo.id>

docs/getting-started/4-ledger.md

Signed-off-by: Timo Glastra <timo@animo.id>

feat(core): support multiple indy ledgers

2e0d353

Signed-off-by: Timo Glastra <timo@animo.id>

TimoGlastra requested review from a team, swcurran and shaangill025 September 26, 2021 21:29

TimoGlastra commented Sep 26, 2021

View reviewed changes

packages/core/src/modules/ledger/services/IndyPoolService.ts Outdated Show resolved Hide resolved

packages/core/src/modules/ledger/__tests__/IndyPoolService.test.ts Outdated Show resolved Hide resolved

berendsliedrecht approved these changes Sep 27, 2021

View reviewed changes

TimoGlastra linked an issue Sep 29, 2021 that may be closed by this pull request

Supporting multiple genesis file #395

Closed

TimoGlastra added 5 commits October 6, 2021 09:30

Merge remote-tracking branch 'upstream/main' into feat/multi-ledger-s…

033f0ca

…upport

fix(core): rework pool selection algorithm

0f6bb0a

Signed-off-by: Timo Glastra <timo@animo.id>

feat(core): add persisted lru cache for did pools

d8e9cf0

Signed-off-by: Timo Glastra <timo@animo.id>

fix(core): update instead of save for cache record

22db2bf

Signed-off-by: Timo Glastra <timo@animo.id>

test(core): agent not initialized

d9fb49d

Signed-off-by: Timo Glastra <timo@animo.id>

JamesKEbert suggested changes Oct 8, 2021

View reviewed changes

TimoGlastra added 3 commits October 9, 2021 14:34

Merge remote-tracking branch 'upstream/main' into feat/multi-ledger-s…

8be264e

…upport

fix: prefer self certifying did

1930f68

Signed-off-by: Timo Glastra <timo@animo.id>

fix: resolved feedback

b43cb3c

Signed-off-by: Timo Glastra <timo@animo.id>

JamesKEbert approved these changes Oct 10, 2021

View reviewed changes

TimoGlastra added 2 commits October 13, 2021 11:23

docs: add multi indy ledger documentation

947606a

Signed-off-by: Timo Glastra <timo@animo.id>

build: exclude all build directories in tsconfig

1b6ae8c

Signed-off-by: Timo Glastra <timo@animo.id>

berendsliedrecht reviewed Oct 13, 2021

View reviewed changes

docs/getting-started/4-ledger.md Outdated Show resolved Hide resolved

TimoGlastra commented Oct 13, 2021

View reviewed changes

docs/getting-started/4-ledger.md Outdated Show resolved Hide resolved

docs: add init config type for ledger docs

558adfa

Signed-off-by: Timo Glastra <timo@animo.id>

TimoGlastra merged commit 47149bc into openwallet-foundation:main Oct 20, 2021

TimoGlastra deleted the feat/multi-ledger-support branch October 20, 2021 09:57

feat(core): support multiple indy ledgers #474

feat(core): support multiple indy ledgers #474

Uh oh!

Conversation

TimoGlastra commented Sep 26, 2021

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Sep 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

MosCD3 commented Sep 27, 2021

Uh oh!

berendsliedrecht left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TimoGlastra commented Sep 30, 2021

Uh oh!

JamesKEbert left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JamesKEbert Oct 8, 2021

Choose a reason for hiding this comment

Uh oh!

TimoGlastra Oct 9, 2021

Choose a reason for hiding this comment

Uh oh!

JamesKEbert Oct 10, 2021

Choose a reason for hiding this comment

Uh oh!

TimoGlastra commented Oct 9, 2021

Uh oh!

JamesKEbert left a comment

Choose a reason for hiding this comment

Uh oh!

JamesKEbert Oct 10, 2021

Choose a reason for hiding this comment

Uh oh!

TimoGlastra Oct 12, 2021

Choose a reason for hiding this comment

Uh oh!

TimoGlastra Oct 13, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Sep 26, 2021 •

edited

Loading