modify the getAllChainConfigs function call from smartcontract with pagination based logic #21
Conversation
| return err | ||
| var allChainConfigInfos []ChainConfigInfo | ||
| pageIndex := uint64(0) | ||
| pageSize := uint64(500) |
There was a problem hiding this comment.
suggestion: should we make this configurable through the function params, or separate it out as a const to another file / top of file?
There was a problem hiding this comment.
will be moving to constant on top of same file
| }, | ||
| &chainConfigInfos, | ||
| ) | ||
| if err != nil { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
it returns anempty array in absence of any data, will never throw errors
there could be scenarios due to rpc issues
internal/reader/home_chain.go
Outdated
| // 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)) | ||
| r.setState(convertOnChainConfigToHomeChainConfig(allChainConfigInfos)) |
There was a problem hiding this comment.
q: why are we skipping state sets when there are no configs? Isn't an empty array a valid state?
There was a problem hiding this comment.
this was an existing approach, yeah in case if the contract function returns empty array
we can still set it but isnt it redundant execution where we can retain initial state as-is
unless there are entries earluer and for some reason the config is removed and state is stale
is this a valid scenario?
There was a problem hiding this comment.
It could save a null pointer reference though down the line - my thinking is that would be less error-prone
There was a problem hiding this comment.
updated to set in case if results are empty or non-empty
commit/plugin_e2e_test.go
Outdated
| @@ -392,6 +392,8 @@ func newNode( | |||
| homeChain, | |||
| ) | |||
|
|
|||
| //ccipReader.On("Sync", mock.Anything).Return(true, nil) | |||
internal/reader/home_chain.go
Outdated
| pageIndex++ | ||
| } | ||
|
|
||
| if len(allChainConfigInfos) >= 0 { |
There was a problem hiding this comment.
suggestion: the check is redundant - the length of an array will always be >= 0 - we can remove it
There was a problem hiding this comment.
removed redundant check
| @@ -16,7 +16,10 @@ import ( | |||
| cciptypes "github.com/smartcontractkit/chainlink-common/pkg/types/ccipocr3" | |||
| ) | |||
|
|
|||
| //go:generate mockery --name HomeChain --output ./mocks/ --case underscore | |||
There was a problem hiding this comment.
q: shouldn't we keep this? Are the mocks generated somewhere else?
There was a problem hiding this comment.
removed the obselete comment
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I think we should keep the go:generate here.
Created the ticket CCIP-2901 that @winder will work on.
There was a problem hiding this comment.
retained the comment back
…tcontract with pagination based logic
…ter the changes for getAllConfigs function
…case of empty results
…n getAllChainConfigs
defistar
force-pushed
the
feature/CCIP-2594-CCIPConfigPagination
branch
from
0ce9024 to
7fcacb7
Compare
| @@ -78,18 +78,28 @@ type nodeSetup struct { | |||
|
|
|||
| func setupHomeChainPoller(lggr logger.Logger, chainConfigInfos []reader.ChainConfigInfo) reader.HomeChain { | |||
There was a problem hiding this comment.
Can we de-dup func setupHomeChainPoller in a follow-up maybe?
There was a problem hiding this comment.
should we create a JIRA task for this to followup? @dimkouv
| @@ -16,7 +16,10 @@ import ( | |||
| cciptypes "github.com/smartcontractkit/chainlink-common/pkg/types/ccipocr3" | |||
| ) | |||
|
|
|||
| //go:generate mockery --name HomeChain --output ./mocks/ --case underscore | |||
| pageIndex := uint64(0) | ||
|
|
||
| for { | ||
| var chainConfigInfos []ChainConfigInfo |
There was a problem hiding this comment.
Can we add something like the following for extra safety?
const pageIndexHardLimit = 100
for {
...
pageIndex++
if pageIndex > pageIndexHardLimit {
return fmt.Errorf("something is wrong!")
}
}
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
We can add some big value for the limit.
There was a problem hiding this comment.
Why do we need a hard limit, as opposed to just giving a warning and fetching everything?
There was a problem hiding this comment.
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)
| &chainConfigInfos, | ||
| ) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
let's wrap that err "get config index:1 pagesize:2: <err>"
There was a problem hiding this comment.
u[pdated with error wrapping
| @@ -19,7 +19,10 @@ import ( | |||
| "github.com/smartcontractkit/chainlink-ccip/pkg/consts" | |||
| ) | |||
|
|
|||
| //go:generate mockery --name HomeChain --output ./mocks/ --case underscore | |||
| const ( | |||
| PageSize = uint64(500) | |||
There was a problem hiding this comment.
no need to export, also better to rename defaultConfigPageSize.
Isn't 500 too much?, I would expect something closer to 10
…ain interface type
…ed query on CCIPConfig
Test Coverage
|
feat: CCIP-2594 modify the getAllChainConfigs function call from smartcontract with pagination based logic