feat: ETHR multiledger suport#174
Conversation
|
Hi @Artemkaaas and @Toktar, I've just opened this PR adding multi-ledger support for the did:ethr method. If there are any adjustments needed (design, configuration format, tests, etc.), I'm happy to update the implementation. Thanks! |
|
Hi @flaviocgarcia78, thank you for this contribution! It is a valuable addition. Appreciate the tests and the comprehensive demo script as well. Before we can move forward with merging, I'd like to flag a few architectural and code quality concerns: Architectural No client caching. get_ledger_for_identifier() and ping_all() instantiate a new LedgerClient on every call. This is expensive (HTTP connections, potential TLS handshake). Looking at Arc I guess, caching was planned, could you add a client cache? Fragile DID parsing. extract_network() takes parts[2] without validating the DID method. It makes sense to check if it's did:ethr and if there's a namespaces VdrError::ClientInvalidResponse is used for configuration errors (missing network, wrong mode). These are semantically different. A dedicated variant like ConfigError or NetworkNotFound would be clearer. Scope This PR mixes several unrelated changes. I'd recommend splitting this into focused PRs, it's very hard to review as-is. Internal infrastructure leak. The .gitlab-ci.yml references gitcorporativo.serpro (internal GitLab) with hardcoded project IDs and SSL certificate workarounds. This shouldn't land in a Hyperledger open-source repo. Committed artifacts. The file vdr/uniffi/--out-dir is a CLI typo artifact, and the 5090-line Swift file should be generated in CI rather than committed. What is the reason of monitoring stack removal from docker-compose? Could you please return it? LedgerMode, LedgerResult, and LedgerClientConfig lack Debug, this makes debugging difficult. And could you please fix linter errors and DCO? Thank you! |
| @@ -0,0 +1,285 @@ | |||
| use std::{ | |||
| collections::HashMap, | |||
| sync::{Arc, Mutex}, | |||
There was a problem hiding this comment.
Mutex is imported but unused. Could you implement client caching please?
| RUSTUP_HOME: "/Users/serpro/.rustup" | ||
| PATH: "/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin:/Users/serpro/.cargo/bin:$PATH" | ||
| before_script: | ||
| - rustup show # debug: garante que o runner enxerga o mesmo rust |
There was a problem hiding this comment.
Could you please use English language for comments?
| needs: ["generate-bindings"] | ||
| script: | ||
| - cd vdr/uniffi | ||
| # Substitui '/' por '-' para nomes de branch seguros |
There was a problem hiding this comment.
Could you please use English language for comments?
| - PACKAGE_VERSION=${CI_COMMIT_REF_NAME//\//-} | ||
| - TAR_NAME=libs-and-bindings-${PACKAGE_VERSION}.tar.gz | ||
| - tar czf $TAR_NAME target out $(test -d wrappers && echo wrappers) | ||
| # Upload direto no script garante que só tenta se o tar existir |
There was a problem hiding this comment.
Could you please use English language for comments?
| build_macos: | ||
| stage: build | ||
| tags: | ||
| - macos # garante que vai rodar só no runner com essa tag |
There was a problem hiding this comment.
Could you please use English language for comments?
| default_network: default_network.unwrap_or("default").to_string(), | ||
| mode: mode.unwrap_or(LedgerMode::ConfigOnly), |
There was a problem hiding this comment.
The magic strings "default" and the implicit ConfigOnly default appear in several places.
| rpc_node: rpc_node.to_string(), | ||
| contract_configs: contract_configs.to_vec(), | ||
| network: network.map(|s| s.to_string()), | ||
| quorum_config: quorum_config.cloned(), |
There was a problem hiding this comment.
quorum_config from LedgerClientConfig is available but not passed to LedgerClient::new() — None is used instead. This looks like an oversight or I missed something
| /// The extracted network name as String | ||
| pub fn extract_network(identifier: &str) -> VdrResult<String> { | ||
| let parts: Vec<&str> = identifier.split(':').collect(); | ||
| if parts.len() < 3 { |
There was a problem hiding this comment.
DID can contain more then 3 prefixes.
| network1 = 'ledger1' | ||
| network2 = 'ledger2' | ||
| network3 = 'ledger3' | ||
| project_root = f"{os.getcwd()}/../../.." |
There was a problem hiding this comment.
Can be an issue if the script is called from a different working directory. os.path.dirname(os.path.abspath(file)) with relative navigation would be more robust.
| print(' Revocation Status List:' + status_list.to_string()) | ||
|
|
||
| if __name__ == "__main__": | ||
| asyncio.get_event_loop().run_until_complete(demo()) |
There was a problem hiding this comment.
deprecated since Python 3.10. asyncio.run(demo()) is the modern replacement.
Signed-off-by: Flavio Caetano Garcia <flavio.garcia@serpro.gov.br>
| config.network.as_deref(), | ||
| None, | ||
| )?; | ||
| Ok(LedgerResult::Client(client)) |
There was a problem hiding this comment.
I think we should use HashMap<String, Arc<LedgerClient>> as a cache because now it does parsing of all contract ABIs and opens a connection on every call of get_ledger_for_identifier, which is called per operation.
| pub network: Option<String>, | ||
| /// Optional quorum configuration, if applicable. | ||
| pub quorum_config: Option<QuorumConfig>, | ||
| } |
There was a problem hiding this comment.
Should it also contain LedgerMode as a property? I see LedgerMode::LedgerClient is hardcoded below, while the actual definition provides two options.
There was a problem hiding this comment.
Good point.
At the moment, I kept LedgerMode hardcoded to LedgerClient in the FFI layer to keep the API simpler and avoid introducing additional enum mappings in UniFFI.
Since the main goal of this PR is to introduce multi-ledger routing and caching, I suggest we keep this behavior for now and handle exposing LedgerMode as a follow-up improvement (it will require proper UniFFI enum support and bindings update).
| configs: HashMap<String, LedgerClientConfig>, | ||
| default_network: String, | ||
| mode: LedgerMode, | ||
| } |
There was a problem hiding this comment.
It looks like LedgerRouter is not integrated into methods like resolve_did, resolve_schema, and so on. They still accept client instances.
I think we should define a common trait for LedgerRouter and LedgerClient, so all existing resolution methods can accept any of the client implementations. At the FFI layer, we can provide duplicate methods if we face an issue.
| LedgerConfiguration(chain_id=config["chainId"], node_address=config["nodeAddress"], contract_configs=contract_configs, network=network1, quorum_config=None), | ||
| LedgerConfiguration(chain_id=config["chainId"], node_address=config["nodeAddress"], contract_configs=contract_configs, network=network2, quorum_config=None), | ||
| LedgerConfiguration(chain_id=config["chainId"], node_address=config["nodeAddress"], contract_configs=contract_configs, network=network3, quorum_config=None) | ||
| ] |
There was a problem hiding this comment.
I do not really think that this demo actually verifies the multi-ledger behaviour correctness. All three configurations use identical chainId and nodeAddress. This is the same single ledger registered under three names.
There was a problem hiding this comment.
You're right — the current demo validates routing behavior rather than real multi-ledger connectivity.
Each network is registered under a distinct name, so the router logic (network resolution and client selection) is exercised, even though the underlying node is the same.
I will clarify this in the demo comments to avoid confusion, and we can extend it later with distinct endpoints if needed.
| receipt = await client.get_receipt(txn_hash) | ||
| print(' Transaction receipt: ' + receipt) | ||
|
|
||
| print("3. Resolve DID Document:" +network1) |
There was a problem hiding this comment.
Label says network1 but client resolves did defined via network2
| needs: ["generate-bindings"] | ||
| variables: | ||
| TWINE_REPOSITORY_URL: "https://gitcorporativo.serpro/api/v4/projects/21675/packages/pypi" | ||
| before_script: |
There was a problem hiding this comment.
It seems to refer to a private corporate GitLab instance (gitcorporativo.serpro) with a hardcoded project ID (21675). It has no relation to this project. This job needs to be deleted.
|
The change added in the PR is definitely valuable - named-network routing for multi-ledger support fills a real gap.
|
Signed-off-by: Flavio Caetano Garcia <flavio.garcia@serpro.gov.br>
b916a42 to
11cd2b5
Compare
First of all, thank you for the opportunity to contribute to the indy-besu project — it’s a great initiative and very interesting work. Special thanks to @Artemkaaas and @Toktar for the thorough review and valuable feedback. It really helped to refine the approach and improve the implementation. I’ve pushed updates addressing the main points discussed. Happy to iterate further if needed. I took a deeper look into integrating the router directly into resolution flows (e.g. resolve_did, resolve_schema), and I agree this is a valuable direction. However, I believe it is safer to keep the current approach for now, where the caller explicitly resolves the LedgerClient via the router: and then passes it to the corresponding operation. The main reason is that not all flows have a reliable identifier to derive the target ledger. For example, in write operations such as schema creation: the schema may not yet have a fully qualified identifier (or network information), making automatic routing ambiguous or error-prone. Because of that, introducing router-based delegation at this stage could lead to implicit behavior and edge cases that are harder to reason about. For this PR, I focused on:
I suggest handling router-based delegation (especially for read/resolve methods) as a follow-up improvement. Happy to extend in that direction in a next PR if that sounds good 👍 — Flavio Garcia |
|
@flaviocgarcia78 #174 (comment) Could you fix markdown linter issues and remove one unprocessed Portuguese comment - https://github.com/hyperledger-indy/indy-besu/pull/174/changes#diff-3739dfc2ecb3a79bfb86811a09028efd7279f2743ecec65bf4917cb2fa6ded08R78 |
Signed-off-by: Flavio Caetano Garcia <flavio.garcia@serpro.gov.br>
11cd2b5 to
39e0031
Compare
@Artemkaaas, I have addressed the requested adjustments:
The branch was updated with the latest changes. |
Summary
This PR introduces support for multi-ledger configuration when resolving the
did:ethrmethod in Indy-Besu.With this change, the resolver can interact with multiple EVM-compatible ledgers instead of relying on a single configured network. This enables broader interoperability and flexibility when working with decentralized identity deployments across different Ethereum-based networks.
Motivation
Currently, the
did:ethrintegration assumes a single ledger configuration. However, real-world deployments often require interacting with multiple networks (e.g., mainnet, testnets, or permissioned EVM chains).Supporting multiple ledgers allows implementers to:
Changes
did:ethrmethodTesting
The following tests were performed:
Compatibility
This change is backward compatible. Existing configurations using a single ledger will continue to function without modification.
Related Work
Related to ongoing work around multi-network interoperability in DID resolution.
Contributor
Contribution by external collaborator.