feat(connector-besu): add WatchSolidityEventV1 endpoint #3844
Conversation
A dedicated endpoint that is purpose-built for solidity events would add a lot of convenience to users. Depends on hyperledger-cacti#3830 Fixes hyperledger-cacti#3825 Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
|
This PR/issue depends on:
|
| } | ||
| } | ||
| }, | ||
| "ViemV2_24_2_EventLog": { |
There was a problem hiding this comment.
I wouldn't use Viem/Web3js name in our return specifications. In case something changes in View output in the future we will have to transform it to match our (older) response specification for API stability. This may be confusing for users who may expect that this structure is pinned to actual View output
There was a problem hiding this comment.
I think @petermetz did this intentionally so that we can replace web3 with viem gradually over time, without breaking functionality of the existing codebase (@petermetz please correct me if I am wrong here).
But yes, if we call it Viem_EventLog instead of ViemV2_24_2_EventLog , it would be cleaner in my opinion (all good otherwise from my end as well)
There was a problem hiding this comment.
@outSH @jagpreetsinghsasan sorry for the slow response! I'll note down here my thoughts, please let me know if based on these it makes sense to keep it as-is or not. If not, I'm fine with changing it, but I wanted to make sure we are on the same page first:
- The probability of breaking changes in the Viem types is 100% as time goes on.
- The probability of security vulnerabilities in Viem that will force us to upgrade is also a 100% as time approaches infinity.
- Based on 1 and 2 it is 100% that we'll have to adjust our types.
- When we are at that juncture, we'll have the choices of a) make a breaking change in the
Viem_EventLogtype which will be more confusing to users or b) avoid a breaking change by havingViemV2_24_2_EventLognow and add a newViemV3_0_0_EventLogand let it be immediately obvious to anyone what exactly happened and why (breaking change in the upstream library and in which specific version, etc.)
I'm not married to option b) so just let me know what do you prefer and I'm happy to update it as such.
There was a problem hiding this comment.
@petermetz true, I just checked the viem gh repo and it seems like they have pretty frequent updates, breaking changes and dependency upgrades. I am thus fine with the current state of the code in this PR.
@outSH what are your thoughts about this?
There was a problem hiding this comment.
@petermetz @jagpreetsinghsasan
When we are at that juncture, we'll have the choices of a) make a breaking change in the Viem_EventLog type which will be more confusing to users or b) avoid a breaking change by having ViemV2_24_2_EventLog now and add a new ViemV3_0_0_EventLog
I'm thinking about a case where we don't actualy change Viem_EventLog, but write an adapter code that will transform the Viem output to backward compatible format, so that our user can use latest library release without security issues and not modify his code at all, but only rely on Cacti API stability. We can of course introduce breaking changes in Viem_EventLog:V2 and remove Viem_EventLog:V1 in next major Cacti release to follow viem implementation in a long run. If we use ViemV2_24_2_EventLog then we're strictly pinned to Viem and user has to update the code every time something in Viem changes, or use older Cacti/Viem version.
I think head/block subscription structure is likely linked to ethereum JSON-RPC and not Viem itself, so the changes we can expect in the future are likely in types or some fields being named differently / marked optional, but the data will be the same between libraries, so I don't expect much work to be done here.
| * @returns The string itself if it was 0x prefixed or a new 0x prefixed version of if it wasn't. | ||
| */ | ||
| export function ensure0xPrefix(maybePrefixed: string): string & `0x${string}` { | ||
| const fn = "ensure-0x-prefix.ts"; |
There was a problem hiding this comment.
Is the .ts there by mistake?
| readonly viemClient: ViemPublicClient; | ||
| } | ||
|
|
||
| export class WatchEventsV1Endpoint { |
There was a problem hiding this comment.
@petermetz, I would just suggest renaming classes/types to something like SolidityEvent or ContractEvent, just like the PR title. This may be a bit misleading since there is usually the possibility of listening for multiple events beyond the ones emitted by contracts (e.g., pending Transactions, new Heads, etc.)
|
@petermetz, we are working on the |
@AndreAugusto11 Sorry for the slow response! I haven't had the time to make the updates requested by @outSH and @jagpreetsinghsasan (I agree with them, I just couldn't find the time). If you'd like to, feel free to update the PR yourself and then re-request their reviews and then we can merge it. |
A dedicated endpoint that is purpose-built for solidity events
would add a lot of convenience to users.
Depends on #3830
Fixes #3825
Signed-off-by: Peter Somogyvari peter.somogyvari@accenture.com
Pull Request Requirements
upstream/mainbranch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.-sflag when usinggit commitcommand. You may refer to this link for more information.Character Limit
A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.