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

OG-515 support pagination for all log queries #637

Merged
merged 10 commits into from
May 14, 2021
Merged

Conversation

forshtat
Copy link
Member

@forshtat forshtat commented May 5, 2021

No description provided.

1. Add 'maxPageSize' as constructor parameter for ContractInteractor.
Not optional as pagination will be needed for all packages (cli, relay &
provider) and tests should be adjusted.

2. Move implementation of pagination support to the Contracts Interactor

3. Fix bug in 'splitRange' that would cause it to lose the last block in
requested range, create test for it.

4. Remove unused config 'relayLookupWindowParts' but keep dynamic adjust

5. Add 'coldRestartLogsFromBlock' config to avoid scanning from block 1

Also, fix or ignore some minor TypeScript errors in tests as they were
previously excluded from compilation.
@forshtat forshtat force-pushed the limited_logging_support branch from 595cf09 to 69a068e Compare May 6, 2021 21:55
return pagesForBlockWindow
}

splitRange (fromBlock: BlockNumber, toBlock: BlockNumber, parts: number): Array<{ fromBlock: BlockNumber, toBlock: BlockNumber }> {
Copy link
Member

Choose a reason for hiding this comment

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

This function is too similar to getPagesForBlockWindow, and they have the same calculation inside - which is generally a bad thing. In this case, you actually have a bug that if you provide to any getPastEvents function in interactor toBlock = 'latest', it will throw in line 441, even though getPagesForBlockWindow supports it.

Copy link
Member Author

Choose a reason for hiding this comment

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

They do have a same calculation but do the opposite things. splitRange has existed before this change and is responsible for translation of 1 range into X smaller ranges. getPagesForBlockWindow determines X. The common code is boilerplate and can be extracted, generally speaking.

@@ -4,7 +4,7 @@
"license": "MIT",
"main": "dist/GsnTestEnvironment.js",
"files": [
"dist/*",
"dist/src/*",
Copy link
Member

Choose a reason for hiding this comment

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

huh?
currently, I nothing in dist/src. all 3 types in this package are in /distt

Copy link
Member Author

Choose a reason for hiding this comment

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

I have changed the tsconfig.js to include test files, so unless we want ./dist/test in @opengsn/dev, they should be excluded.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, you made 17 writing errors in this sentence! 😂

@@ -202,6 +202,7 @@ contract('Paymaster Commitment', function ([_, relayOwner, relayManager, relayWo

}, sharedRelayRequestData, chainId, forwarderInstance)
const gasAndDataLimits = await paymasterContract.getGasAndDataLimits()
// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

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

can you use ts-error with explicit message, so we know what is wrong?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not always. Some errors do not manifest while running via ts-node and then it failed for me. Might be fixed already, but needs separate verification.

@@ -24,15 +24,15 @@ services:
restart: always
ports: [ '8080:80' ] #bypass https-portal
# add line for each relay
command:
- gsn1
command:
Copy link
Member

Choose a reason for hiding this comment

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

why this space changes?

shahafn
shahafn previously approved these changes May 14, 2021
@shahafn shahafn merged commit 66328e9 into master May 14, 2021
@shahafn shahafn deleted the limited_logging_support branch May 14, 2021 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants