Move read capability to capability repo by ettec · Pull Request #21 · smartcontractkit/capabilities

ettec · 2024-09-26T09:52:30Z

No description provided.

nolag · 2024-09-26T13:19:31Z

+	lggr logger.Logger
+
+	capabilities.CapabilityInfo
+	capabilities.Validator[RequestConfig, Input, capabilities.CapabilityResponse]


If you use the json schema, you don't need the validator. You can simply marshal the object to json and back. This does the same thing, but needs to build up the schema, whereas using the generated classes doesn't require us to build the schema back up.

it seems simpler to just use the validator.Validate method rather than have to go values->type->json explicitly in the code? though not as efficient.

The validator does the same thing internally, but less efficiently. I'm ok with it for now, I'll add a Validate() method on the structs later and we can switch to that because it won't need to sterilize and deserialize.

nolag · 2024-09-26T13:22:27Z

+func (r *ReadContractAction) RegisterToWorkflow(ctx context.Context, request capabilities.RegisterToWorkflowRequest) error {
+	// Do Nothing
+	return nil
+}
+
+func (r *ReadContractAction) UnregisterFromWorkflow(ctx context.Context, request capabilities.UnregisterFromWorkflowRequest) error {
+	// Do Nothing
+	return nil
+}


Don't you need to start the contract reader on register and stop it on unregister? Otherwise, if it's meant to read events, it won't be listening for them.

its not meant to read events, @kidambisrinivas is working on a trigger capability to do that

I'm not talking about triggering on events, the chain reader can read a lastest value from events as well. If you don't want to support that (which would be a regression compared to the underlying chain reader), you'll need to verify that the user doesn't request it somehow because the underlying chain reader will just allow it and return nothing otherwise.

CC: @ilija42 to explain the use case.

Baseline is since this s a chain agnostic component its important to Start and Close the service to give room for handling catching up/starting up or doing whatever else is needed to make some data searchable. Looks like ReadContractAction uses GetLatestValue which is meant to read the latest state, on EVM this state can be contract state or latest emitted log. For this reason you need to start the service to kickstart EVM Log Poller. I think Aptos will do something similar and I imagine that Solana will also need this wiggle room.

ic, ok I guess that would have been caught during the integration testing (still a WIP), I didn't get that from the docs on the Chainreader interface

updated the capability to start/stop contract readers

DeividasK

This is missing project.json

jmank88 · 2024-09-30T17:51:40Z

This is missing project.json

Will that trigger the CI checks?

Addressed

nolag · 2024-10-01T17:41:40Z

@@ -0,0 +1,49 @@
+Capability to read a contract on a given chain.  The chain is configured by providing a json string in the config parameter of the standard capability, for example:


This readme won't reflect a workflow, they don't call execute themselves. They will use the generated code. For now, it's a lot better than nothing.

nolag · 2024-10-01T17:43:11Z

+	lggr logger.Logger
+
+	capabilities.CapabilityInfo
+	capabilities.Validator[readcontractcap.Config, readcontractcap.Input, capabilities.CapabilityResponse]


You could just serialize and deserialize the config, and input. That would validate them for you. Not blocking, but it's slightly more efficient. I'm likely going to add a Validate() method in future that would make it so you don't need to serialize and deserialize.

nolag · 2024-10-01T17:47:17Z

+func (r *ReadContractAction) RegisterToWorkflow(ctx context.Context, request capabilities.RegisterToWorkflowRequest) error {
+	// Do Nothing - there are no resources managed by this capability that match the lifecycle of a workflow
+	return nil
+}
+
+func (r *ReadContractAction) UnregisterFromWorkflow(ctx context.Context, request capabilities.UnregisterFromWorkflowRequest) error {
+	// Do Nothing - there are no resources managed by this capability that match the lifecycle of a workflow
+	return nil
+}


GetLatestValue in contract reader can be set to use the latest event emitted. Register and unregister may need to deal with that. We don't need it for Swift's PoC, and it's not clear how a handoff would work if a workflow changes, but a TODO would be good.

For now, it's probably ok to just ignore this case. @ilija42 and @jmank88 any objections?

@nolag rgd workflow modification, why would unregister/register not be called in this case? If they are, given this would be relatively infrequent (I assume), why is handover a concern?

nolag · 2024-10-01T17:49:34Z

+// ServiceCache is a cache that evicts and closes services after a configurable period of inactivity once a minimum size is reached.
+// It is safe for concurrent use.  The ServiceCache is responsible for starting and stopping the services it caches and itself should
+// be closed to ensure all services are stopped.
+type ServiceCache[I comparable, S services.Service] struct {


This won't work once the chain reader deals with events for get latest value. It seems like a pre-mature optimization. Why not just cache it on register and remove it on unregister workflow?

I was 50/50 on which approach to use tbh, the above had the advantage of sharing readers across workflows. However, given what you say above rgd event reading possibly needing this I'll update to use workflow caching.

cedric-cordenier · 2024-10-04T10:18:10Z

+	contractReaders *ServiceCache[string, ContractReader]
+}
+
+type Relayer interface {


I'm not sure this will work: in Go interfaces aren't "mapped" transitively, i.e. something which satisfies this interface:

type Relayer interface {
NewContractReader() BigInterface
}

does not match this interface:

type Relayer interface {
NewContractReader() SmallerSubsetInterface
}

I wrap the relayer, otherwise it would not compile:

`type readContractRelayer struct {
relayer core.Relayer
}

func (r *readContractRelayer) NewContractReader(ctx context.Context, contractReaderConfig []byte) (actions.ContractReader, error) {
return r.relayer.NewContractReader(ctx, contractReaderConfig)
}`

Cheeky :) Yeah that'll do it

I'm not sure if this is helpful, but you could capture the return type as a generic type param:

type Relayer[CR ContractReader] interface { NewContractReader() CR }

ettec force-pushed the move-read-capability-to-capability-repo branch 2 times, most recently from 6f93ac0 to 1e47751 Compare September 26, 2024 10:15

ettec marked this pull request as ready for review September 26, 2024 10:21

ettec force-pushed the move-read-capability-to-capability-repo branch 2 times, most recently from fb07ea9 to 4740ab7 Compare September 26, 2024 10:26

nolag reviewed Sep 26, 2024

View reviewed changes

DeividasK previously requested changes Sep 26, 2024

View reviewed changes

jmank88 reviewed Oct 1, 2024

View reviewed changes

Comment thread readcontract/.mockery.yaml Outdated

jmank88 reviewed Oct 1, 2024

View reviewed changes

Comment thread readcontract/action/read_contract_action_test.go

jmank88 reviewed Oct 1, 2024

View reviewed changes

Comment thread readcontract/action/read_contract_action_test.go Outdated

nolag force-pushed the move-read-capability-to-capability-repo branch from 7fd9fb3 to 1877ffa Compare October 1, 2024 14:34

ettec force-pushed the move-read-capability-to-capability-repo branch 2 times, most recently from 627218e to 4a65957 Compare October 1, 2024 17:32

nolag reviewed Oct 1, 2024

View reviewed changes

Comment thread readcontract/action/read_contract_action.go

ettec and others added 12 commits October 3, 2024 10:05

read contract capability

7d30a9e

expirable cache

3ea3122

introduce service cache

f417f2e

update output

ad0e336

config

e18b06e

json schema

f0d8fc5

json schema and generate code

592b707

use generated types in readcontract capability

6ba2698

update tests and doc to use generated types

157582e

Add s in Outputs and Inputs, and add project.json

16c1367

Chain reader isn't a set capability, it needs to be chosen

18e1c48

CI fixes and review comments, update capability version number

e0461b6

ettec force-pushed the move-read-capability-to-capability-repo branch from 6dae1b7 to e0461b6 Compare October 3, 2024 09:07

cedric-cordenier reviewed Oct 4, 2024

View reviewed changes

Comment thread readcontract/main.go Outdated

cedric-cordenier reviewed Oct 4, 2024

View reviewed changes

Comment thread readcontract/action/read_contract_action.go Outdated

cedric-cordenier reviewed Oct 4, 2024

View reviewed changes

ettec added 2 commits October 4, 2024 12:41

review comments

29aa0fe

build

bf3f591

cedric-cordenier approved these changes Oct 4, 2024

View reviewed changes

ettec added this pull request to the merge queue Oct 4, 2024

Merged via the queue into main with commit d8f38c8 Oct 4, 2024

ettec deleted the move-read-capability-to-capability-repo branch October 4, 2024 14:19

		@@ -0,0 +1,49 @@
		Capability to read a contract on a given chain. The chain is configured by providing a json string in the config parameter of the standard capability, for example:

Uh oh!

Conversation

ettec commented Sep 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ettec Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilija42 Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DeividasK left a comment

Choose a reason for hiding this comment

Uh oh!

jmank88 commented Sep 30, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ettec Oct 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ettec Sep 26, 2024 •

edited

Loading

ilija42 Sep 27, 2024 •

edited

Loading

ettec Oct 2, 2024 •

edited

Loading