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

feat: Changing GateSeal address #535

Merged
merged 2 commits into from
Apr 26, 2024
Merged

Conversation

sergeyWh1te
Copy link
Contributor

No description provided.

@sergeyWh1te sergeyWh1te requested a review from a team as a code owner April 24, 2024 15:22
@sergeyWh1te sergeyWh1te linked an issue Apr 24, 2024 that may be closed by this pull request
TheDZhon
TheDZhon previously approved these changes Apr 25, 2024
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 for the effort and changed brought!
Left some nitpicks and questions here and there.

Have only a single consideration about old instance tests — is it worth doing it this way comparing with #532?

ethereum-steth/.nvmrc Outdated Show resolved Hide resolved
ethereum-steth/src/app.ts Outdated Show resolved Hide resolved
ethereum-steth/src/entity/events.ts Show resolved Hide resolved
const EXITBUS_ORACLE_ADDRESS: string = '0x0de4ea0184c2ad0baca7183356aea5b8d5bf5c6e'
const GATE_SEAL_FACTORY_ADDRESS: string = '0x6c82877cac5a7a739f16ca0a89c0a328b8764a24'
const WITHDRAWALS_VAULT_ADDRESS: string = '0xb9d7934878b5fb9610b3fe8a5e441e8fad7e293f'
const EL_REWARDS_VAULT_ADDRESS: string = '0x388c818ca8b9251b393131c08a736a67ccb19297'

// const for backward functional test capability
// https://github.com/lidofinance/alerting-forta/issues/533
export const GATE_SEAL_DEFAULT_ADDRESS_BEFORE_26_APR_2024: string = '0x1ad5cb2955940f998081c1ef5f5f00875431aa90'
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure that we should store this constant tbh

I mean, the approach used in #532 was less intrusive for the codebase and less verbose 🤔

@sergeyWh1te
Copy link
Contributor Author

sergeyWh1te commented Apr 25, 2024

I'll try to describe some actions in the codebase:

  • } catch (e) {
    To according eslint rules this "catch" could not be empty block. Yes, sounds resealable. Different side of coin - ethers.utils.Interface([listenAbiEvent]).parseLog throws Error when current log does not have compatibility with listenAbiEvent.

We have list of events by one address like on a picture. But code could not know what exactly Event is placed in array of logs before parsing each one.
Screenshot 2024-04-25 at 12 01 45

  • I am not sure that we should store this constant tbh
    No worry. I'll remove it after Friday deploy. I made it for temp back capability with older gateseal address. Also, you could find that only that GATE_SEAL_DEFAULT_ADDRESS_BEFORE_26_APR_2024 directly is exported from constants.ts

  • Removed tests that I have wrote.
    Those tests were written for testing filterLog function provided by FortaSDK. This commit removes this dependency for our custom logic. Also, new code is fully test covered.

Screenshot 2024-04-25 at 12 20 40 Screenshot 2024-04-25 at 12 18 00

@TheDZhon
Copy link
Contributor

I'll remove it after Friday deploy. I made it for temp back capability with older gateseal address. Also, you could find that only that GATE_SEAL_DEFAULT_ADDRESS_BEFORE_26_APR_2024 directly is exported from constants.ts

I am just wondering how we would shape the tests for the GateSeal expiration alerts :)

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.

👍

@sergeyWh1te sergeyWh1te merged commit 6af771d into main Apr 26, 2024
5 checks passed
@sergeyWh1te sergeyWh1te deleted the 533-changing-gateseal-address branch April 26, 2024 08:30
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.

Changing GateSeal address
2 participants