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

Jump to bottom

Small fixes and refactoring across multiple bots #551

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Small fixes and refactoring across multiple bots #551

wants to merge 6 commits into from

Conversation

arwer13
Copy link
Contributor

@arwer13 arwer13 commented May 28, 2024 •
edited
Loading

  • unify address format in filtering to prevent possible if and address hardcoded in non-lowercase format
  • prettier
  • fix ethereum-steth test (the trailing spaces in expected string were trimmed)
  • any --> unkown

@arwer13 arwer13 changed the title Fix/most bots small fixes Small fixes and refactoring across multiple bots May 28, 2024
@arwer13
fix(tests): ethereum-steth test if trailing spaces trimmed
f6c6467
@arwer13
fix(multiple bots): unify address format in event filtering
b15400c 
to prevent possible incorrect logic if a hardcoded address is specified
in checksum format
@arwer13 arwer13 force-pushed the fix/most-bots-small-fixes branch from 407b109 to b15400c Compare May 28, 2024 15:28
@arwer13 arwer13 marked this pull request as ready for review May 28, 2024 15:30
@arwer13 arwer13 requested a review from a team as a code owner May 28, 2024 15:30
@arwer13 arwer13 requested a review from sergeyWh1te May 28, 2024 15:31
TheDZhon
TheDZhon previously approved these changes May 28, 2024
View reviewed changes
Copy link
Contributor

@TheDZhon TheDZhon left a comment

Choose a reason for hiding this comment

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

Thank you!

@TheDZhon
Copy link
Contributor

though, please pay attention that one of the l2-bridge-base tests has failed to pass

@arwer13
Copy link
Contributor Author

arwer13 commented May 28, 2024

@TheDZhon they are broken in main as well a few other tests
and tests in general are not stable and fail from time to time due to CI issues

sergeyWh1te
sergeyWh1te reviewed May 29, 2024
View reviewed changes
l2-bridge-ethereum/src/agent.ts Outdated
@@ -152,7 +152,7 @@ const handleBlock: HandleBlock = async (
}),
);

runs.forEach((r: PromiseSettledResult<any>, index: number) => {
runs.forEach((r: PromiseSettledResult<unknown>, index: number) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it really better ?=)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it fixes linter warnings. And essentially a more restrictive type

l2-bridge-zksync/src/services/event_watcher.ts
@@ -27,14 +28,15 @@ export class EventWatcher {
const logIndexToLog = new Map<number, Log>()

for (const l2Log of l2Logs) {
addresses.add(l2Log.address.toLowerCase())
addresses.add(formatAddress(l2Log.address))
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to use .toLowerCase() instead of forta.formatAddress.

Motivation: we want to drop out forta-sdk dependency at all and forta.fortmatAddress makes a string in lowerCase too inside itself

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to keep it as it is. At least for now to move forward.

Converting using a dedicated function would at least allow to easily identify all the contexts and replace easily by either changing import or searching/replacing be.

@sergeyWh1te
Copy link
Contributor

sergeyWh1te commented May 29, 2024 •
edited
Loading

@TheDZhon they are broken in main as well a few other tests and tests in general are not stable and fail from time to time due to CI issues

I see that CI faced with network errors.
Screenshot 2024-05-29 at 11 33 36

@sergeyWh1te sergeyWh1te force-pushed the fix/most-bots-small-fixes branch 3 times, most recently from e6759f3 to 0aca5a2 Compare May 29, 2024 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sergeyWh1te sergeyWh1te sergeyWh1te left review comments

@TheDZhon TheDZhon TheDZhon left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@arwer13 @TheDZhon @sergeyWh1te