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

TOML config: implement remaining interfaces #7523

Merged
merged 5 commits into from
Oct 3, 2022

Conversation

jmank88
Copy link
Contributor

@jmank88 jmank88 commented Sep 24, 2022

https://app.shortcut.com/chainlinklabs/story/33615/create-new-implementation-of-chainscopedconfig-generalconfig-interfaces-that-sources-config-from-a-config-toml-file
https://app.shortcut.com/chainlinklabs/story/37975/chains-nodes-should-be-read-from-the-config-interface

  • added BlockBackfillDepth & BlockBackfillSkip fallback values
  • added v2.ChainScoped to implement config.ChainScopedConfig with a gencfg.BasicConfig and v2.EVMConfig
  • refactored monolithic setFrom() methods to be per-struct
  • expanded unique name & URL validation cross chains of the same type
  • updated graphql & node controllers to use Name instead of ID from database (still unqiue)
  • added TOML Chain and ChainSet variants
  • added immutable TOML ORM variants
  • pull core defaults from docs
  • test EVM fallback defaults against docs
  • rename GeneralOnlyConfig to BasicConfig and move some GlobalConfig methods
  • added standard ErrUnsupported error ("unsupported with config v2")
  • cleaner validation error format

Configuration Transition

- subgraph names are packages
- thick lines indicate control flow
- dotted lines indicate implicit interface implementation
- regular w/ dot indicate implementation types
flowchart LR

    subgraph cmd
    
        subgraph cmd/app
            NewApp([func NewApp])     
        end
    
        cli>$ chainlink node start]
        
        RunNode([func Client.RunNode])
        
        NewApplication([func NewApplication])
            
        cli == 1. Before ==> NewApp
        cli == 2. Action ==> RunNode
        RunNode ==> NewApplication
    
    end
    
    toml{{TOML?}}    
    
    subgraph services/chainlink
        
        Config[[Config]]
        
        NewTOMLGeneralConfig([func NewTOMLGeneralConfig])
        
        generalConfig --o Config
        
        NewTOMLGeneralConfig --> generalConfig
       
    end
    
    subgraph config
    
        BasicConfig(BasicConfig)
        
        NewGeneralConfig([func NewGeneralConfig])
        
        generalConfig2[generalConfig]
        
        NewGeneralConfig --> generalConfig2
    
        subgraph config/v2
        
            Core[[Core]]
        
        end
    
    end
    
    Config --o Core
    
    NewApp ==> toml
    toml == yes ==> NewTOMLGeneralConfig
    toml == no ==> NewGeneralConfig
    generalConfig -.-> BasicConfig
    generalConfig2 -.-> BasicConfig
    
    
    subgraph chains/evm
    
        LoadChainSet([func LoadChainSet])
        tomlChain{{TOML?}}
        LoadChainSet ==> tomlChain
    
        subgraph chains/evm/config  
        
            NewChainScopedConfig([func NewChainScopedConfig])
        
            ChainScopedOnlyConfig(ChainScopedOnlyConfig)
            
            chainScopedConfig
            
            NewChainScopedConfig --> chainScopedConfig
            
            chainScopedConfig -.-> ChainScopedOnlyConfig
            
            subgraph chains/evm/config/v2 
            
                NewTOMLChainScopedConfig([func NewTOMLChainScopedConfig])
            
                ChainScoped
                
                NewTOMLChainScopedConfig --> ChainScoped
                
                ChainScoped -.-> ChainScopedOnlyConfig
                
                EVMConfig[[EVMConfig]]
                
            end
        
        end
        
        tomlChain == no ==>NewChainScopedConfig
        tomlChain == yes ==>NewTOMLChainScopedConfig
        Config --o EVMConfig
    end
    
    chainScopedConfig --o generalConfig
    ChainScoped --o generalConfig2 
    
    NewApplication ==> LoadChainSet
    
Loading

https://github.com/smartcontractkit/chainlink/blob/sc-33615-chain-scoped-config/core/services/chainlink/CONFIG.md

TODO

  • Non-EVM chain defaults
  • match legacy validation
  • Expose configv2 endpoint in graphql #7490
  • Audit Logs Rebase #7443
  • add standard deprecation comments, e.g. // https://app.shortcut.com/chainlinklabs/story/33622/remove-legacy-config (cannot use proper // Deprecation yet tag since staticcheck blocks usages)
  • "EvmGasBumpTxDepth should default to EvmMaxInFlightTransactions and may not be set larger than this" to follow-up
  • graphql: did I break the UI? yes, but then I fixed it
operator-ui screenshots

Screenshot from 2022-10-03 07-40-19
Screenshot from 2022-10-03 07-57-37
Screenshot from 2022-10-03 07-57-30

Requires:

@github-actions
Copy link
Contributor

I see that you haven't updated any CHANGELOG files. Would it make sense to do so?

@jmank88 jmank88 changed the title WIP TOML config: implement remaining interfaces Sep 24, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Sep 24, 2022

Solana Smoke Test Results

1 tests   1 ✔️  4m 39s ⏱️
1 suites  0 💤
1 files    0

Results for commit d241683.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 24, 2022

EVM Smoke Test Results

75 tests   33 ✔️  6m 22s ⏱️
  1 suites  42 💤
  1 files      0

Results for commit d241683.

♻️ This comment has been updated with latest results.

@jmank88 jmank88 force-pushed the sc-33615-chain-scoped-config branch 14 times, most recently from 3305697 to 68797d2 Compare September 27, 2022 22:42
samsondav
samsondav previously approved these changes Sep 28, 2022
core/chains/chain_set.go Outdated Show resolved Hide resolved
core/chains/evm/chain_set.go Outdated Show resolved Hide resolved
core/chains/evm/config/v2/chain_scoped.go Show resolved Hide resolved
core/chains/evm/types/types.go Show resolved Hide resolved
- dotted lines indicate implicit interface implementation
- regular w/ dot indicate implementation types

```mermaid
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice! Was this generated or did you write it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✋ 🖌️

type Pyroscope struct {
AuthToken *string
//TODO enabled?
AuthToken *string //TODO move to secrets?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vyzaldysanchez @samsondav This should probably be moved to secrets.toml, no?

pinebit
pinebit previously approved these changes Sep 30, 2022
vyzaldysanchez
vyzaldysanchez previously approved these changes Sep 30, 2022
@jmank88 jmank88 dismissed stale reviews from vyzaldysanchez and pinebit via 7a2ffc0 September 30, 2022 13:44
@jmank88 jmank88 force-pushed the sc-33615-chain-scoped-config branch 2 times, most recently from 7a2ffc0 to c987050 Compare September 30, 2022 19:21
@@ -29,7 +29,7 @@ func NewNodes(nodes []types.Node) []*NodeResolver {

// ID resolves the node's unique identifier.
func (r *NodeResolver) ID() graphql.ID {
return int32GQLID(r.node.ID)
return graphql.ID(r.node.Name) //TODO change on UI side too?
Copy link
Contributor

Choose a reason for hiding this comment

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

This may require changes in the UI, depending on what's being done there with this value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can optionally drop the display column since it is redundant now, but the rest I think I will be able to patch up.
Screenshot from 2022-10-03 07-40-19

@jmank88 jmank88 force-pushed the sc-33615-chain-scoped-config branch 2 times, most recently from f20fc66 to c3b9909 Compare October 3, 2022 13:02
- added `v2.ChainScoped` to implement `config.ChainScopedConfig` with a `gencfg.BasicConfig` and `v2.EVMConfig`
- refactored monolithic `setFrom()` methods to be per-struct
- expanded unique name & URL validation cross chains of the same type
- updated graphql & node controllers to use `Name` instead of `ID` from database (still unqiue)
- added TOML Chain and ChainSet variants
- added immutable TOML ORM variants
- pull core defaults from docs
- test EVM fallback defaults against docs
- rename `GeneralOnlyConfig` to `BasicConfig` and move some `GlobalConfig` methods
- added standard `ErrUnsupported` error ("unsupported with config v2")
- cleaner validation error format
@jmank88 jmank88 force-pushed the sc-33615-chain-scoped-config branch from c3b9909 to 204e762 Compare October 3, 2022 13:08
@jmank88 jmank88 force-pushed the sc-33615-chain-scoped-config branch from 204e762 to d241683 Compare October 3, 2022 13:29
@@ -90,16 +90,23 @@ type ChainScopedOnlyConfig interface {

//go:generate mockery --name ChainScopedConfig --output ./mocks/ --case=underscore
type ChainScopedConfig interface {
config.GeneralConfig
config.BasicConfig
Copy link
Collaborator

Choose a reason for hiding this comment

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

A better name 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't recall if I explained the nuance of this change anywhere, but for posterity: I moved some methods between the two interfaces to align the abstractions with how we were using them. The old sets were intuitive for humans, but this way some uses could be reduced to just one interface or the other. (and we avoid another set of unimplemented/panicing v2 methods)

@jmank88 jmank88 merged commit c06dac4 into develop Oct 3, 2022
@jmank88 jmank88 deleted the sc-33615-chain-scoped-config branch October 3, 2022 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review PR is ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants