[KS-182] keystone: Refactor write_capability + add ChainWriter #13259

archseer · 2024-05-20T15:09:20Z

Use chainwriter
Move evm specific init under the evm relayer
Check if report already delivered via chainreader

core/capabilities/targets/write_target_test.go

archseer · 2024-05-20T15:12:05Z

core/capabilities/targets/write_target.go

+	cr.Bind(ctx, []commontypes.BoundContract{{
+		Address: config.ForwarderAddress().String(),
+		Name:    "forwarder",
+		// Pending: false, // ???


@nolag I couldn't find any documentation on what this flag does

If you are reading a contract method it doesn't really do anything.

If you are reading a log event it translated to confirmations Finalized/Unfinalized, will be changed soon to make more sense

core/capabilities/targets/write_target.go

…tegy Move chain writer send strategy to be config driven

cl-sonarqube-production · 2024-05-31T03:47:01Z

Quality Gate passed

Issues
4 New issues
4 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
82.4% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

bolekk · 2024-05-31T04:40:59Z

core/capabilities/targets/write_target.go

-		// SignalCallback:   true, TODO: add code that checks if a workflow id is present, if so, route callback to chainwriter rather than pipeline
-	}
-	tx, err := txm.CreateTransaction(ctx, req)
+	txID, err := uuid.NewUUID() // TODO(archseer): it seems odd that CW expects us to generate an ID, rather than return one


This TODO is not really actionable, I'd remove it.

bolekk · 2024-05-31T04:42:12Z

core/capabilities/targets/write_target.go

-	if err != nil {
+	// Check whether value was already transmitted on chain
+	queryInputs := struct {
+		Receiver            string


You might need to add ReportID here but let's merge this PR first and then handle new meta fields separately.

bolekk · 2024-05-31T04:43:10Z

core/capabilities/targets/write_target.go

-	txm := cap.chain.TxManager()
+func success() <-chan capabilities.CapabilityResponse {
+	callback := make(chan capabilities.CapabilityResponse)
+	go func() {


You can create a channel with size 1 and avoid this goroutine.

bolekk · 2024-05-31T04:44:03Z

core/capabilities/targets/write_target.go

+func success() <-chan capabilities.CapabilityResponse {
+	callback := make(chan capabilities.CapabilityResponse)
+	go func() {
+		callback <- capabilities.CapabilityResponse{}


Don't we want to put some info in the response? For example whether we attempted transmission or not. Maybe not important atm but let's consider it in the future.

bolekk · 2024-05-31T04:47:44Z

core/services/relay/evm/chain_writer.go

+		_, cancel := context.WithTimeout(context.Background(), time.Second)
+		defer cancel()
+
+		// TODO(nickcorin): Add shutdown steps here.


are there any?

bolekk · 2024-05-31T04:48:45Z

core/services/relay/evm/evm.go

+	}
+
+	// Initialize write target capability if configuration is defined
+	if chain.Config().EVM().ChainWriter().ForwarderAddress() != nil {


We should probably rename this to "KeystoneForwarderAddress" to not be confused with Operator Forwarders.

bolekk · 2024-05-31T04:50:56Z

core/services/relay/evm/write_target.go

+	}
+	cw := NewChainWriterService(lggr.Named("ChainWriter"), chain.Client(), chain.TxManager(), chainWriterConfig)
+
+	return targets.NewWriteTarget(lggr, name, cr, cw, config.ForwarderAddress().String()), nil


Can we avoid having two objects with the same name "WriteTarget"?

bolekk · 2024-05-31T04:51:22Z

core/services/relay/evm/write_target.go

+						ChainSpecificName: "report",
+						Checker:           "simulate",
+						FromAddress:       config.FromAddress().Address(),
+						GasLimit:          200_000,


This should be in TOML config (separate PR)

archseer requested review from bolekk, nolag and DeividasK May 20, 2024 15:09

archseer had a problem deploying to sdlc May 20, 2024 15:09 — with GitHub Actions Failure

archseer commented May 20, 2024

View reviewed changes

core/capabilities/targets/write_target_test.go Outdated Show resolved Hide resolved

archseer force-pushed the cap-idempotent-writes branch from 573aaf0 to 7c17dc2 Compare May 20, 2024 15:11

archseer temporarily deployed to sdlc May 20, 2024 15:11 — with GitHub Actions Inactive

archseer commented May 20, 2024

View reviewed changes

ilija42 reviewed May 20, 2024

View reviewed changes

core/capabilities/targets/write_target.go Show resolved Hide resolved

bolekk reviewed May 22, 2024

View reviewed changes

core/capabilities/targets/write_target.go Outdated Show resolved Hide resolved

archseer had a problem deploying to sdlc May 24, 2024 06:38 — with GitHub Actions Failure

nickcorin added 10 commits May 24, 2024 17:13

evm: Add a stub chainwriter impl

e515e44

evm: Fix config parameter

6b1d8f2

evm: Change the chainwriter receiver name

a9ed95e

evm: Remove the chain writer interface to reference chainlink-common

8d7680c

evm: Update common dep, and fix signature

c37c142

go.sum: Run gomodtidy

65db322

.changeset: Add a changeset

2377b72

evm: Pseudo-implement the submit transaction method on chainwriter

ffafc46

evm: Add txm dependency to chainwriter

3879bb4

evm: Use the txm param properly

0ffbe37

archseer force-pushed the cap-idempotent-writes branch 2 times, most recently from 117a932 to 7546304 Compare May 27, 2024 05:39

archseer temporarily deployed to sdlc May 27, 2024 05:39 — with GitHub Actions Inactive

archseer force-pushed the cap-idempotent-writes branch from 7546304 to 1a66e05 Compare May 27, 2024 06:18

archseer temporarily deployed to sdlc May 27, 2024 06:18 — with GitHub Actions Inactive

archseer force-pushed the cap-idempotent-writes branch from 1a66e05 to bf6d0ee Compare May 28, 2024 07:32

archseer temporarily deployed to sdlc May 28, 2024 07:32 — with GitHub Actions Inactive

archseer mentioned this pull request May 28, 2024

[BCI-3271] Initial refactor of Keystone Write Capability for ChainWriter #13299

Closed

archseer force-pushed the cap-idempotent-writes branch from bf6d0ee to 86ed2b2 Compare May 29, 2024 06:06

archseer and others added 12 commits May 31, 2024 11:53

integration-tests: go mod tidy

5fdd136

evm: Only initialize write target if config actually present

4daba0f

more tidy, fix last lint

8b683f6

evm: Move send strategy to be config driven

1125a9d

evm: Add some todos back

7da8ad5

.changeset: Add a changeset

3d0bf8b

write_target: Mock out Config before NewRelayer is called

514a7fe

Regenerate mocks with right version

3abb386

Merge remote-tracking branch 'origin/develop' into cap-idempotent-writes

4df3c26

Merge pull request #13369 from smartcontractkit/feat/config-send-stra…

78b22de

…tegy Move chain writer send strategy to be config driven

fix evm tests

8c504d1

goimports

17b34f1

archseer force-pushed the cap-idempotent-writes branch from 0668ae4 to 17b34f1 Compare May 31, 2024 02:55

archseer temporarily deployed to sdlc May 31, 2024 02:56 — with GitHub Actions Inactive

fix evm tests

a38547a

archseer temporarily deployed to sdlc May 31, 2024 03:29 — with GitHub Actions Inactive

archseer requested a review from cedric-cordenier May 31, 2024 03:47

bolekk reviewed May 31, 2024

View reviewed changes

bolekk approved these changes May 31, 2024

View reviewed changes

archseer requested a review from ilija42 May 31, 2024 07:12

archseer enabled auto-merge May 31, 2024 07:12

nickcorin approved these changes May 31, 2024

View reviewed changes

archseer added this pull request to the merge queue May 31, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 31, 2024

archseer added this pull request to the merge queue May 31, 2024

Merged via the queue into develop with commit 76dbe19 May 31, 2024
109 checks passed

archseer deleted the cap-idempotent-writes branch May 31, 2024 09:16

github-actions bot mentioned this pull request May 31, 2024

[DO NOT MERGE] Changeset Release Preview - v2.20.0 #13148

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[KS-182] keystone: Refactor write_capability + add ChainWriter #13259

[KS-182] keystone: Refactor write_capability + add ChainWriter #13259

archseer commented May 20, 2024 •

edited

Loading

archseer May 20, 2024

ilija42 May 20, 2024

ilija42 May 20, 2024 •

edited

Loading

cl-sonarqube-production bot commented May 31, 2024

bolekk May 31, 2024

bolekk May 31, 2024

bolekk May 31, 2024

bolekk May 31, 2024

bolekk May 31, 2024

bolekk May 31, 2024

bolekk May 31, 2024

bolekk May 31, 2024

[KS-182] keystone: Refactor write_capability + add ChainWriter #13259

[KS-182] keystone: Refactor write_capability + add ChainWriter #13259

Conversation

archseer commented May 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilija42 May 20, 2024 • edited Loading

Choose a reason for hiding this comment

cl-sonarqube-production bot commented May 31, 2024

Quality Gate passed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

archseer commented May 20, 2024 •

edited

Loading

ilija42 May 20, 2024 •

edited

Loading