-
Notifications
You must be signed in to change notification settings - Fork 32
Refine regex for max block count extraction and add test #735
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
base: master
Are you sure you want to change the base?
Conversation
Updated regex to capture maximum block count from error messages more accurately - namely update to Nodies provider. Added a new test case for extracting this specific error response.
| let regex_result = | ||
| Regex::new(r".* (max: |allowed for your plan: |is limited to |block range limit \(|exceeds max block range )(?P<max_block_count>\d+).*") | ||
| .expect("Invalid regex"); | ||
| Regex::new(r".* (max: |allowed for your plan: |is limited to |block range limit \(|exceeds max block range |maximum allowed is )(?P<max_block_count>\d+).*") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So now this is a six-fold regular expression. I think that's probably confusing enough: if there turns out to be a seventh option here, we should redesign this to be a list of single-option regexes. We might even want to put the list in the database so that we can distribute little scripts that customers can run to update it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats a very good idea.
I added the sixth, so our original approach was getting stacked many-fold.
Ill write a card and we can see if the adding to db is a lot of extra work
|
I have QA'd this myself (CC: @Syther007 ) running on this build. Node is handling the parsing and max-block-range change successfully in a test run (using base-mainnet) However, it seems the Nodies endpoint still returns the error when max-block-range is set to 20,000 as indicated in their error: I have passed to Nodies team to diagnose to see if there is any further aspects we have to consider. Also, I managed to nail a test scan of 19,999 blocks and that worked successfully with Nodies without error. So Node side testing is a pass, but before we push to production I want to check with Nodies provider with our results |
|
I have discovered something interesting which maybe has been interfering with the max-block-range logic and handling with the block scans. After testing and meeting with our RPC blockchain service provider, we realized that when a max block range is parsed out and added to our config, the next scan is performed across the limited range but the starting block number counts as "1 block" and the end range block number is simply start block number + the max range. that actually makes the scan range max-block-range +1 and the error will repeat because its over the max range by 1 block, since it is including the starting block number of the scan. In the test example: The block range of |
…faceWeb3 Adjusted the end block marker calculation to correctly account for inclusive ranges by subtracting 1 from the scan range. T
…rface. Adjusted expected new start block values to ensure accurate transaction retrieval and logging.
|
QA Pass on Windows build: |
Updated regex to capture maximum block count from error messages more accurately - namely update to Nodies provider. Added a new test case for extracting this specific error response.
Closes #734