Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create a common folder #9

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Create a common folder #9

wants to merge 3 commits into from

Conversation

pavel-raykov
Copy link

@pavel-raykov pavel-raykov commented Jan 16, 2025

Moved files from chainlink/common to chains with the following caveats:

  • chainlink/common/types files go directly to chains, other folders (fees, headtracker, txmgr) are copied as folders
  • a new module github.com/smartcontractkit/chainlink-framework/chains has been created
  • all the imports have been changed as follows:
    • "github.com/smartcontractkit/chainlink/v2/common/types" -> "github.com/smartcontractkit/chainlink-framework/chains"
    • "github.com/smartcontractkit/chainlink/v2/common/fees" -> "github.com/smartcontractkit/chainlink-framework/chains/fees"
    • "github.com/smartcontractkit/chainlink/v2/common/headtracker" -> "github.com/smartcontractkit/chainlink-framework/chains/headtracker"
    • "github.com/smartcontractkit/chainlink/v2/common/txmgr/types" -> "github.com/smartcontractkit/chainlink-framework/chains/txmgr/types" (txmgrtypes alias is collapsed whenever possible)

A one-to-one diff is available here https://github.com/smartcontractkit/chainlink/pull/15960/files

@pavel-raykov pavel-raykov requested a review from jmank88 January 16, 2025 18:58
github.com/prometheus/client_golang v1.20.5
github.com/shopspring/decimal v1.4.0
github.com/smartcontractkit/chainlink-common v0.4.0
github.com/smartcontractkit/chainlink-framework/multinode v0.0.0-20250115203616-a2ea5e50b260
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This dependency is unfortunate. Although it looks like the only usages is for a SendTxReturnCode which has a binary state. I bet we could eliminate it. 🤔

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, could you please clarify why you think this dependency is "unfortunate"? Do you mean that in general we want the code dependencies to be as simple as possible and this one complicates it, or is there any other reason to eliminate it?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dependencies between modules in the same repo can be a headache, since devs want to be able to update them both atomically, which forces us to use hacky local replaces. In this case, I think we should just switch to using a bool instead of a custom type with two values for success/fatal.

Copy link
Author

@pavel-raykov pavel-raykov Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but what about txmgr/broadcaster.go and txmgr/confirmer.go that have a full-fledged switch logic for SendTxReturnCode? Do you think we should try changing them to also use boolean?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, there are actually many different kinds of meaningful codes. This module was only referencing two of them though. I don't think a bool is the answer, but there might be a different way to approach it, like by making the aggegrate func pluggable so that this module only deals with an opaque RESULT any. We can figure this out in a follow-up.

@pavel-raykov pavel-raykov marked this pull request as ready for review January 17, 2025 16:05
@pavel-raykov pavel-raykov requested a review from a team as a code owner January 17, 2025 16:05
@pavel-raykov pavel-raykov disabled auto-merge January 17, 2025 16:57
@jmank88
Copy link

jmank88 commented Jan 17, 2025

LGTM, but let's see the chainlink/ PR build/test with this before merging 👍

@pavel-raykov
Copy link
Author

LGTM, but let's see the chainlink/ PR build/test with this before merging 👍

This is the corresponding draft smartcontractkit/chainlink#15972 (I can update it once this is submitted)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants