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.

Sign up for GitHub

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

modify the getAllChainConfigs function call from smartcontract with pagination based logic #21

Closed
wants to merge 18 commits into from
Closed

modify the getAllChainConfigs function call from smartcontract with pagination based logic #21

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
74e7469
feat: CCIP-2594 modify the getAllChainConfigs function call from smar…
Jul 4, 2024
c11a9aa
fix: CCIP-2594 fix commit and execute plugin e2e and unit-tests to ca…
Jul 12, 2024
c27f997
fix: CCIP-2594 fix failing paginagtion tests on ccipConfig
Jul 16, 2024
a693127
feat: CCIP-2594 config-pagination output can be set to state even in …
Jul 16, 2024
7fcacb7
fix: CCIp-2594 fix failing homePoller Tests which asserts the calls o…
Jul 17, 2024
a7e28ab
Merge branch 'ccip-develop' into feature/CCIP-2594-CCIPConfigPagination
defistar Jul 29, 2024
a555251
fix: CCIP-2594 fix dependency issue for tests
Jul 29, 2024
505afe3
fix: CCIP-2594 fix dependency issue for tests
Jul 29, 2024
1c94481
fix: CCIP-2594 commit plugin tests
Jul 29, 2024
170ba79
fix: CCIP-2594 commit plugin fix for ccip-config paginated query test
Jul 30, 2024
61bd0c9
Merge branch 'ccip-develop' into feature/CCIP-2594-CCIPConfigPagination
defistar Jul 30, 2024
87753bb
fix: CCIP-2594 remove redundant length check on queried CCIPConfig
Jul 30, 2024
b7ba839
fix: CCIP-2594 Pagination hardLimits for pageIndex and PageSize
Jul 30, 2024
73a0f3a
fix: CCIP-2594 go:generate mock generator command added at the HomeCh…
Jul 30, 2024
5bbd178
fix: CCIP-2594 dev comments for max cap on pageIndex used for paginat…
Jul 31, 2024
b958ca9
fix: CCIP-2594 pagination caps updated
Aug 1, 2024
d665203
fix: CCIP-2594 CCIPConfig Pagination refactoring conflict resolution …
Aug 11, 2024
595aa83
fix: CCIP-2594 Conflict resolution with base branch
Aug 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions commit/plugin_e2e_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,18 +403,28 @@ func newNode(

func setupHomeChainPoller(lggr logger.Logger, chainConfigInfos []reader.ChainConfigInfo) reader.HomeChain {
homeChainReader := mocks.NewContractReaderMock()
var firstCall = true
homeChainReader.On(
"GetLatestValue",
mock.Anything,
consts.ContractNameCCIPConfig,
consts.MethodNameGetAllChainConfigs,
mock.Anything,
mock.Anything,
mock.MatchedBy(func(input map[string]interface{}) bool {
_, pageIndexExists := input["pageIndex"]
_, pageSizeExists := input["pageSize"]
return pageIndexExists && pageSizeExists
}),
mock.Anything,
).Run(
func(args mock.Arguments) {
arg := args.Get(5).(*[]reader.ChainConfigInfo)
*arg = chainConfigInfos
if firstCall {
*arg = chainConfigInfos
firstCall = false
} else {
*arg = []reader.ChainConfigInfo{} // return empty for other pages
}
}).Return(nil)

homeChain := reader.NewHomeChainConfigPoller(
Expand Down
14 changes: 12 additions & 2 deletions execute/plugin_e2e_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,18 +78,28 @@ type nodeSetup struct {

func setupHomeChainPoller(lggr logger.Logger, chainConfigInfos []reader.ChainConfigInfo) reader.HomeChain {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we de-dup func setupHomeChainPoller in a follow-up maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we create a JIRA task for this to followup? @dimkouv

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, create one pls

homeChainReader := mocks.NewContractReaderMock()
var firstCall = true
homeChainReader.On(
"GetLatestValue",
mock.Anything,
consts.ContractNameCCIPConfig,
consts.MethodNameGetAllChainConfigs,
mock.Anything,
mock.Anything,
mock.MatchedBy(func(input map[string]interface{}) bool {
_, pageIndexExists := input["pageIndex"]
_, pageSizeExists := input["pageSize"]
return pageIndexExists && pageSizeExists
}),
mock.Anything,
).Run(
func(args mock.Arguments) {
arg := args.Get(5).(*[]reader.ChainConfigInfo)
*arg = chainConfigInfos
if firstCall {
*arg = chainConfigInfos
firstCall = false
} else {
*arg = []reader.ChainConfigInfo{} // return empty for other pages
}
}).Return(nil)

homeChain := reader.NewHomeChainConfigPoller(
Expand Down
49 changes: 35 additions & 14 deletions internal/reader/home_chain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,10 @@ import (
"github.com/smartcontractkit/chainlink-ccip/pkg/consts"
)

//go:generate mockery --name HomeChain --output ./mocks/ --case underscore
Copy link

Choose a reason for hiding this comment

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

q: shouldn't we keep this? Are the mocks generated somewhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed the obselete comment

Copy link
Contributor

Choose a reason for hiding this comment

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

@defistar wdym?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there was a comment added to use mockery command. that was not supposed to be here.it must be in readme doc.
so deleted that from code @dimkouv

Copy link

Choose a reason for hiding this comment

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

https://vektra.github.io/mockery/latest/#why-mockery - the purpose of the comment is a directive to generate mockery mocks from my understanding (see //go:generate mockery --name DB example) - adding it to the README would not apply any change. Could you elaborate on why you think it's not supposed to be here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should keep the go:generate here.
Created the ticket CCIP-2901 that @winder will work on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

retained the comment back

const (
PageSize = uint64(500)
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to export, also better to rename defaultConfigPageSize.
Isn't 500 too much?, I would expect something closer to 10

Copy link
Contributor Author

Choose a reason for hiding this comment

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

u[pdated

)

type HomeChain interface {
GetChainConfig(chainSelector cciptypes.ChainSelector) (ChainConfig, error)
GetAllChainConfigs() (map[cciptypes.ChainSelector]ChainConfig, error)
Expand Down Expand Up @@ -115,24 +118,42 @@ func (r *homeChainPoller) poll() {
}

func (r *homeChainPoller) fetchAndSetConfigs(ctx context.Context) error {
var chainConfigInfos []ChainConfigInfo
err := r.homeChainReader.GetLatestValue(
ctx,
consts.ContractNameCCIPConfig,
consts.MethodNameGetAllChainConfigs,
primitives.Unconfirmed,
nil,
&chainConfigInfos,
)
if err != nil {
return err
var allChainConfigInfos []ChainConfigInfo
pageIndex := uint64(0)

for {
var chainConfigInfos []ChainConfigInfo
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add something like the following for extra safety?

const pageIndexHardLimit = 100

for {
   ...
   pageIndex++

   if pageIndex > pageIndexHardLimit {
      return fmt.Errorf("something is wrong!")
   }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added extra safety with hardCap for pageIndex
its included in for loop and once for loop exits we check if the pageIndex > pageIndexHardLimit
log a warn message

but wouldnt this cap the max chains we can extract the config from?

@dimkouv

Copy link
Contributor

Choose a reason for hiding this comment

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

We can add some big value for the limit.

Copy link

Choose a reason for hiding this comment

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

Why do we need a hard limit, as opposed to just giving a warning and fetching everything?

Copy link

Choose a reason for hiding this comment

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

I think it would be painful to debug if we don't return all pages and just emit a warning (as opposed to either returning all pages, or throwing an exception)

err := r.homeChainReader.GetLatestValue(
ctx,
consts.ContractNameCCIPConfig,
consts.MethodNameGetAllChainConfigs,
primitives.Unconfirmed,
map[string]interface{}{
"pageIndex": pageIndex,
"pageSize": PageSize,
},
&chainConfigInfos,
)
if err != nil {
Copy link

Choose a reason for hiding this comment

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

q: when do we get an error? Is it always guaranteed to return an empty array if I am accessing out of bounds pages (e.g. 10 pages beyond what is available)?

Should we add a comment that we always expect a result to be returned?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it returns anempty array in absence of any data, will never throw errors
there could be scenarios due to rpc issues

return err
Copy link
Contributor

Choose a reason for hiding this comment

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

let's wrap that err "get config index:1 pagesize:2: <err>"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

u[pdated with error wrapping

}

if len(chainConfigInfos) == 0 {
break
}

allChainConfigInfos = append(allChainConfigInfos, chainConfigInfos...)
pageIndex++
}
if len(chainConfigInfos) == 0 {

r.setState(convertOnChainConfigToHomeChainConfig(allChainConfigInfos))

if len(allChainConfigInfos) == 0 {
// That's a legitimate case if there are no chain configs on chain yet
r.lggr.Warnw("no on chain configs found")
return nil
}
r.setState(convertOnChainConfigToHomeChainConfig(chainConfigInfos))

return nil
}

Expand Down
11 changes: 8 additions & 3 deletions internal/reader/home_chain_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import (

"github.com/smartcontractkit/chainlink-common/pkg/logger"
cciptypes "github.com/smartcontractkit/chainlink-common/pkg/types/ccipocr3"

"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
)
Expand Down Expand Up @@ -117,6 +116,7 @@ func Test_PollingWorking(t *testing.T) {
}

homeChainReader := mocks.NewContractReaderMock()
var firstCall = true
homeChainReader.On(
"GetLatestValue",
mock.Anything,
Expand All @@ -128,14 +128,19 @@ func Test_PollingWorking(t *testing.T) {
).Run(
func(args mock.Arguments) {
arg := args.Get(5).(*[]ChainConfigInfo)
*arg = onChainConfigs
if firstCall {
*arg = onChainConfigs
firstCall = false
} else {
*arg = []ChainConfigInfo{} // return empty for other pages
}
}).Return(nil)

defer homeChainReader.AssertExpectations(t)

var (
tickTime = 2 * time.Millisecond
totalSleepTime = tickTime * 4
totalSleepTime = tickTime * 0
)

configPoller := NewHomeChainConfigPoller(
Expand Down
2 changes: 1 addition & 1 deletion internal/reader/mocks/home_chain.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading