Cross-spork client checks for boundaries#263
Conversation
WalkthroughThe recent changes encompass improvements in error logging, refinement of client setup for cross-spork functionality, and enhancements to spork configuration robustness. These updates include detailed error logging enhancements, the introduction of a new Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (4)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sumtests/go.modis excluded by!**/*.modtests/go.sumis excluded by!**/*.sum
Files selected for processing (8)
- api/api.go (2 hunks)
- bootstrap/bootstrap.go (7 hunks)
- services/ingestion/engine.go (4 hunks)
- services/ingestion/subscriber_test.go (2 hunks)
- services/requester/cross-spork_client.go (5 hunks)
- services/requester/cross-spork_client_test.go (1 hunks)
- services/requester/requester.go (10 hunks)
- services/testutils/mock_client.go (1 hunks)
Files skipped from review due to trivial changes (1)
- api/api.go
Additional comments not posted (25)
services/ingestion/subscriber_test.go (3)
11-11: Ensure the new importtestutilsis utilized effectively in the test setup.
26-27: Refactoring to useSetupClientForRangeimproves modularity and reusability of client setup.
29-29: Good use ofSetupClientForRangefor setting up thecurrentClient. This maintains consistency in client setup across tests.services/testutils/mock_client.go (3)
1-1: Proper package declaration fortestutils.
12-18: Introduction ofMockClientwith functional fields allows flexible mocking of client behaviors, enhancing testability.
41-84:SetupClientForRangefunction is well-implemented, providing a convenient way to configure mock clients for specific block height ranges. This is crucial for testing different scenarios in a controlled environment.services/requester/cross-spork_client_test.go (4)
1-1: Proper package declaration forrequester.
15-31: Comprehensive tests forsporkClient.containsmethod. These tests ensure that the method accurately checks if a given height is within the client's range.
34-67: Tests forsporkClientsadding and validation logic are thorough, ensuring that clients are added correctly and that their ranges are continuous.
70-126: Extensive testing ofCrossSporkClientfunctionality, covering scenarios from client retrieval based on height to error handling for out-of-range queries. These tests are crucial for ensuring the robustness of cross-spork client operations.services/requester/cross-spork_client.go (4)
5-5: Importing necessary packages for error handling and logging.
15-26: Definition ofsporkClientstruct andcontainsmethod. This setup is essential for determining if a height is within the spork's range, which is a core functionality of theCrossSporkClient.
30-84: Implementation ofsporkClientsmethodsaddandget. These methods are well-designed to manage spork clients effectively, ensuring that they are added correctly and can be retrieved based on block height.
Line range hint
95-209: TheCrossSporkClientstruct and its methods are implemented to handle different spork clients based on block height. This is crucial for the system's ability to handle requests across different sporks seamlessly.bootstrap/bootstrap.go (3)
60-81: Integration ofCrossSporkClientin the bootstrap process. This change is crucial for enabling the system to handle cross-spork capabilities right from the start.
Line range hint
125-155: The changes tostartIngestionto useCrossSporkClientensure that the ingestion process can handle data across different sporks. This is vital for the robustness and flexibility of the system.
Line range hint
191-209: Updates tostartServerto utilizeCrossSporkClient. These changes are essential for ensuring that the server can handle requests across different sporks, enhancing the system's scalability and reliability.services/ingestion/engine.go (2)
128-128: Enhanced logging inprocessEventsprovides better visibility into the ingestion process, which is crucial for monitoring and debugging.
179-189: The detailed logging inindexBlockimproves traceability of block processing, which is essential for understanding the flow of data through the system.services/requester/requester.go (6)
91-98: The update to use*CrossSporkClientin theEVMstruct and its constructor aligns with the PR's objectives to handle cross-spork clients effectively.
165-165: Enhanced error logging inSendRawTransactionimproves visibility and is beneficial for debugging transaction issues.
272-277: Enhanced error logging inGetBalanceimproves diagnostics and is crucial for troubleshooting issues in financial applications.
305-309: Updated error logging inGetNonceenhances operational monitoring and debugging capabilities.
343-348: Updated error handling and logging inCallenhance diagnostics and traceability, which are crucial for blockchain operations.
362-362: Logging the result inCallmethod is a good practice for traceability and debugging.
| go func(id flow.Identifier) { | ||
| res, _ := e.client.GetTransactionResult(context.Background(), id) | ||
| if res != nil && res.Error != nil { | ||
| e.logger.Error(). | ||
| Str("flow-id", id.String()). | ||
| Str("events", fmt.Sprintf("%v", res.Events)). | ||
| Msg("flow transaction executed successfully") | ||
| }(flowTx.ID()) | ||
| } | ||
| Err(res.Error). | ||
| Msg("flow transaction failed to execute") | ||
| return | ||
| } | ||
|
|
||
| e.logger.Info(). | ||
| Str("flow-id", id.String()). | ||
| Str("events", fmt.Sprintf("%v", res.Events)). | ||
| Str("status", res.Status.String()). | ||
| Msg("flow transaction executed successfully") | ||
| }(flowTx.ID()) |
There was a problem hiding this comment.
Asynchronous logging of transaction results in signAndSend is a good practice for non-blocking operations. However, consider adding nil checks before dereferencing res to avoid potential runtime panics.
if res != nil {
if res.Error != nil {
e.logger.Error().Str("flow-id", id.String()).Err(res.Error).Msg("flow transaction failed to execute")
} else {
e.logger.Info().Str("flow-id", id.String()).Str("events", fmt.Sprintf("%v", res.Events)).Str("status", res.Status.String()).Msg("flow transaction executed successfully")
}
}
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| type mockClient struct { |
| @@ -0,0 +1,84 @@ | |||
| package testutils | |||
|
|
||
| e.logger.Debug(). | ||
| go func(id flow.Identifier) { | ||
| res, _ := e.client.GetTransactionResult(context.Background(), id) |
There was a problem hiding this comment.
note: fetching the tx result immediately doesn't tell you if the tx actually executed successfully. it will always return return pending
There was a problem hiding this comment.
removed for #264
this change doesn't really belong in this PR
| // make sure each last height is one smaller than next range first height | ||
| for i := 0; i < len(lasts)-1; i++ { | ||
| if lasts[i]+1 != firsts[i+1] { | ||
| return false |
There was a problem hiding this comment.
it'd be helpful to log which blocks were missing between which clients
There was a problem hiding this comment.
how would you report that? I guess changing this to return an error? but then what is the method? mustBeContinuius feels weird?
There was a problem hiding this comment.
maybe checkContinuous?
This is fine, it will just be a bit of manual checking for the operator if the list is not contiguous
| firsts := make([]uint64, len(*s)) | ||
| lasts := make([]uint64, len(*s)) | ||
|
|
||
| for i, c := range *s { |
There was a problem hiding this comment.
nit: I think you could simplify this logic by sorting the list of clients first, then iterating over the list once.
There was a problem hiding this comment.
Good idea, added.
There was a problem hiding this comment.
Actionable comments posted: 6
Outside diff range and nitpick comments (4)
tests/web3js/eth_logs_filtering_test.js (1)
Line range hint
57-57: Use strict equality===for comparisons to avoid type coercion issues.- .filter(v => v.A == repeatA) + .filter(v => v.A === repeatA)tests/web3js/eth_streaming_test.js (1)
Line range hint
31-43: Avoid usingasyncfunctions as Promise executors to prevent potential issues with unhandled promise rejections.- let doneBlocks = new Promise(async (res, rej) => { + let doneBlocks = new Promise((res, rej) => { - let doneTxs = new Promise(async (res, rej) => { + let doneTxs = new Promise((res, rej) => { - let doneAddressLogs = new Promise(async (res, rej) => { + let doneAddressLogs = new Promise((res, rej) => {Also applies to: 48-58, 63-75
tests/web3js/eth_streaming_filters_test.js (2)
Line range hint
10-10: Replaceletwithconstfor variables that are assigned only once to ensure immutability where possible.- let contractDeployment = await helpers.deployContract("storage") + const contractDeployment = await helpers.deployContract("storage") - let contractAddress = contractDeployment.receipt.contractAddress + const contractAddress = contractDeployment.receipt.contractAddress - let contractDeployment2 = await helpers.deployContract("storage") + const contractDeployment2 = await helpers.deployContract("storage") - let contractAddress2 = contractDeployment2.receipt.contractAddress + const contractAddress2 = contractDeployment2.receipt.contractAddress - let repeatA = 10 + const repeatA = 10 - let ws = new Web3("ws://127.0.0.1:8545") + const ws = new Web3("ws://127.0.0.1:8545") - let storageContract = new ws.eth.Contract(storageABI, contractAddress) + const storageContract = new ws.eth.Contract(storageABI, contractAddress) - let storageContract2 = new ws.eth.Contract(storageABI, contractAddress) + const storageContract2 = new ws.eth.Contract(storageABI, contractAddress) - let calculatedEvent = storageContract.events.Calculated + const calculatedEvent = storageContract.events.Calculated - let rawSubscribe = filter => ws.eth.subscribe('logs', filter) + const rawSubscribe = filter => ws.eth.subscribe('logs', filter)Also applies to: 35-35, 40-40, 42-42, 48-48, 70-70, 71-71, 74-74, 75-75, 77-77, 86-86, 88-88, 89-89, 90-90, 92-92, 97-97, 153-153, 164-164
Line range hint
38-38: Use strict inequality!==for comparisons to avoid type coercion issues.- if (allLogs[i].returnValues != undefined) { + if (allLogs[i].returnValues !== undefined) {
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
tests/go.modis excluded by!**/*.modtests/go.sumis excluded by!**/*.sum
Files selected for processing (6)
- services/requester/cross-spork_client.go (5 hunks)
- services/requester/cross-spork_client_test.go (1 hunks)
- services/requester/requester.go (10 hunks)
- tests/web3js/eth_logs_filtering_test.js (2 hunks)
- tests/web3js/eth_streaming_filters_test.js (1 hunks)
- tests/web3js/eth_streaming_test.js (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- services/requester/cross-spork_client_test.go
- services/requester/requester.go
Additional Context Used
Biome (45)
tests/web3js/eth_logs_filtering_test.js (8)
57-57: Use === instead of ==.
== is only allowed when comparing againstnull
9-9: This let declares a variable that is only assigned once.
10-10: This let declares a variable that is only assigned once.
12-12: This let declares a variable that is only assigned once.
22-22: This let declares a variable that is only assigned once.
31-31: This let declares a variable that is only assigned once.
45-45: This let declares a variable that is only assigned once.
59-59: This let declares a variable that is only assigned once.
tests/web3js/eth_streaming_filters_test.js (19)
38-38: Use !== instead of !=.
!= is only allowed when comparing againstnull
10-10: This let declares a variable that is only assigned once.
35-35: This let declares a variable that is only assigned once.
40-40: This let declares a variable that is only assigned once.
42-42: This let declares a variable that is only assigned once.
48-48: This let declares a variable that is only assigned once.
70-70: This let declares a variable that is only assigned once.
71-71: This let declares a variable that is only assigned once.
74-74: This let declares a variable that is only assigned once.
75-75: This let declares a variable that is only assigned once.
77-77: This let declares a variable that is only assigned once.
86-86: This let declares a variable that is only assigned once.
88-88: This let declares a variable that is only assigned once.
89-89: This let declares a variable that is only assigned once.
90-90: This let declares a variable that is only assigned once.
92-92: This let declares a variable that is only assigned once.
97-97: This let declares a variable that is only assigned once.
153-153: This let declares a variable that is only assigned once.
164-164: This let declares a variable that is only assigned once.
tests/web3js/eth_streaming_test.js (18)
31-43: Promise executor functions should not be
async.
48-58: Promise executor functions should not be
async.
63-75: Promise executor functions should not be
async.
11-11: This let declares a variable that is only assigned once.
12-12: This let declares a variable that is only assigned once.
14-14: This let declares a variable that is only assigned once.
23-23: This let declares a variable that is only assigned once.
30-30: This let declares a variable that is only assigned once.
31-31: This let declares a variable that is only assigned once.
32-32: This let declares a variable that is only assigned once.
47-47: This let declares a variable that is only assigned once.
48-48: This let declares a variable that is only assigned once.
49-49: This let declares a variable that is only assigned once.
61-61: This let declares a variable that is only assigned once.
63-63: This let declares a variable that is only assigned once.
64-64: This let declares a variable that is only assigned once.
80-80: This let declares a variable that is only assigned once.
83-83: This let declares a variable that is only assigned once.
| const web3 = conf.web3 | ||
|
|
||
| it('emit logs and retrieve them using different filters', async() => { | ||
| setTimeout(() => process.exit(1), 19*1000) // hack if the ws connection is not closed |
There was a problem hiding this comment.
Consider using Mocha's built-in timeout mechanism instead of a manual setTimeout for test timeouts.
| it('emit logs and retrieve them using different filters', async() => { | ||
| setTimeout(() => process.exit(1), 19*1000) // hack if the ws connection is not closed | ||
|
|
||
| let deployed = await helpers.deployContract("storage") |
There was a problem hiding this comment.
Replace let with const for variables that are assigned only once to ensure immutability where possible.
- let deployed = await helpers.deployContract("storage")
+ const deployed = await helpers.deployContract("storage")
- let contractAddress = deployed.receipt.contractAddress
+ const contractAddress = deployed.receipt.contractAddress
- let repeatA = 10
+ const repeatA = 10
- let latest = await web3.eth.getBlockNumber()
+ const latest = await web3.eth.getBlockNumber()
- let events = await deployed.contract.getPastEvents('Calculated', {
+ const events = await deployed.contract.getPastEvents('Calculated', {Also applies to: 10-10, 12-12, 22-22, 31-31, 45-45, 59-59
Committable suggestion was skipped due low confidence.
| const timeout = 30 // test timeout seconds | ||
|
|
||
| it('streaming of logs using filters', async() => { | ||
| setTimeout(() => process.exit(1), (timeout-1)*1000) // hack if the ws connection is not closed |
There was a problem hiding this comment.
Consider using Mocha's built-in timeout mechanism instead of a manual setTimeout for test timeouts.
| it('streaming of logs using filters', async() => { | ||
| setTimeout(() => process.exit(1), (timeout-1)*1000) // hack if the ws connection is not closed | ||
|
|
||
| let deployed = await helpers.deployContract("storage") |
There was a problem hiding this comment.
Replace let with const for variables that are assigned only once to ensure immutability where possible.
- let deployed = await helpers.deployContract("storage")
+ const deployed = await helpers.deployContract("storage")
- let contractAddress = deployed.receipt.contractAddress
+ const contractAddress = deployed.receipt.contractAddress
- let repeatA = 10
+ const repeatA = 10
- let ws = new Web3("ws://127.0.0.1:8545")
+ const ws = new Web3("ws://127.0.0.1:8545")
- let blockCount = 0
+ const blockCount = 0
- let blockHashes = []
+ const blockHashes = []
- let doneBlocks = new Promise(async (res, rej) => {
+ const doneBlocks = new Promise((res, rej) => {Also applies to: 12-12, 14-14, 23-23, 30-30, 31-31, 32-32, 47-47, 48-48, 49-49, 61-61, 63-63, 64-64, 80-80, 83-83
Committable suggestion was skipped due low confidence.
| var ErrOutOfRange = errors.New("height is out of range for provided spork clients") | ||
|
|
||
| type sporkClient struct { | ||
| firstHeight uint64 | ||
| lastHeight uint64 | ||
| client access.Client | ||
| } |
There was a problem hiding this comment.
The implementation of CrossSporkClient is robust with appropriate error handling and continuity checks. Consider enhancing the error messages and logs for better traceability.
- return fmt.Errorf("could not get latest height using the spork client: %w", err)
+ return fmt.Errorf("error retrieving latest block header from spork client: %w", err)
- return fmt.Errorf("could not get node info using the spork client: %w", err)
+ return fmt.Errorf("error retrieving node version info from spork client: %w", err)
- c.logger.Debug().Msg("using previous spork client")
+ c.logger.Debug().Str("client", client.ID()).Msg("selected spork client based on height")Also applies to: 23-26, 28-54, 56-65, 67-77, 88-122, 130-202
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| var ErrOutOfRange = errors.New("height is out of range for provided spork clients") | |
| type sporkClient struct { | |
| firstHeight uint64 | |
| lastHeight uint64 | |
| client access.Client | |
| } | |
| var ErrOutOfRange = errors.New("height is out of range for provided spork clients") | |
| type sporkClient struct { | |
| firstHeight uint64 | |
| lastHeight uint64 | |
| client access.Client | |
| } | |
| func (c *CrossSporkClient) someFunction() error { | |
| // Example function to show the application of the diff | |
| if err != nil { | |
| return fmt.Errorf("error retrieving latest block header from spork client: %w", err) | |
| } | |
| if err != nil { | |
| return fmt.Errorf("error retrieving node version info from spork client: %w", err) | |
| } | |
| c.logger.Debug().Str("client", client.ID()).Msg("selected spork client based on height") | |
| return nil | |
| } |
| } | ||
|
|
||
| it('streaming of logs using filters', async() => { | ||
| setTimeout(() => process.exit(1), (timeout-1)*1000) // hack if the ws connection is not closed |
There was a problem hiding this comment.
Consider using Mocha's built-in timeout mechanism instead of a manual setTimeout for test timeouts.
2 participants
Closes: #231
Description
This PR improves how cross-spork clients are handled. It adds a range for each client defined by first and last height in that spork, which enables more safety, first it enabled for all the clients to be checked if they create a continuous range of heights (without missing heights), secondly and more importantly, the requests error out if the height requested is out of range, which drops requests made to the AN and thus lower traffic and load.
Implementing this is important because there was a lot of requests for heights out of range being forwarded to ANs.
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
Improvements
Testing