Add IBlockchainApiManager for on-chain interactions
#1623
Conversation
Pull Request Test Coverage Report for Build 9446016581Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9451506530Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9463145202Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9464947500Details
💛 - Coveralls |
| destroyBlockchainApi(chainId: string): void { | ||
| if (this.blockchainApiMap?.[chainId]) { | ||
| delete this.blockchainApiMap[chainId]; | ||
| } |
There was a problem hiding this comment.
This is great. In fact, being strict we should implement the same destroy mechanism in transaction-api.manager.ts, as the Transaction Service URLs can also change (even is it's pretty unusual). Now it requires a CGW restart to get a new configuration for a given TransactionApi datasource. What do you think?
There was a problem hiding this comment.
Agree that it's unusual but I think it would be a good pattern to copy. We could even consider creating a specific interface for these managers that we implement in both.
There was a problem hiding this comment.
I refactored the TransactionApiManager and BalancesApiManager to follow the same pattern with a common interface in #1640. When that is merged, I will propagate the changes into the BlockchainApiManager as well.
| getClient(): PublicClient { | ||
| return createPublicClient({ | ||
| chain: this.getChain(), | ||
| transport: http(), | ||
| }); | ||
| } |
There was a problem hiding this comment.
I'm not totally against this, but wanted to known the approach better: why do we expose the PublicClient outside this class? Wouldn't it be more inline with the other datasource classes (*-api.service.ts) to just expose the functions itself (even we are mostly wrapping the client in this case)?
I mean, instead of the having the clients of this class doing:
const blockchainClient = blockChainApi.getClient();
const blockNumber = await blockchainClient.getBlockNumber();
They would do something like:
const blockNumber = await blockchainApi.getBlockNumber();
Exposing the PublicClient seems a bit like defeating the purpose of having a datasource, which in my opinion has as one of its main benefits the encapsulation of the underlying implementation. What do you think?
There was a problem hiding this comment.
Good point. IBlockchainApi is essentially redundant (it was a remanant from trialling the implementation). I removed it in e38f109 as per your suggestion.
| constructor( | ||
| private readonly configurationService: IConfigurationService, | ||
| private readonly chain: DomainChain, | ||
| public readonly destroyClient: (chainId: string) => void, |
There was a problem hiding this comment.
What's the purpose of this function? I understand BlockchainApiManager.destroyBlockchainApi is intended to delete the BlockChainApi entry in the map, but in this class this function doesn't do anything, right? It's intended to release some resources?
There was a problem hiding this comment.
It is a remnant from trialling the implementation. I removed it in e38f109.
Pull Request Test Coverage Report for Build 9467806260Details
💛 - Coveralls |
iamacook
changed the title
BlockchainApi with associated managerIBlockchainApiManager for on-chain interactions
Summary
Smart contract wallets use EIP-1271 to sign messages. As we progress with SiWe-based authenticated, we will also need to add support for verification of messages signed with EIP-1271. Currently, our SiWe implementation assumes that signed messages come only from EOAs. In order to support verification for the above, we will need to make on-chain requests.
This adds a new
IBlockchainManager, returning a client per chain. It can be used as a basis for future on-chain interactions, e.g. verification of EIP-1271 signatures.It is important to note that w need a specific API key for the project.
Changes
IBlockchainApiManagerand implementation with test coverageCHAIN_UPDATE