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

chore: check if tx index enabled #301

Merged
merged 8 commits into from
Jan 27, 2025
Merged

chore: check if tx index enabled #301

merged 8 commits into from
Jan 27, 2025

Conversation

Lazar955
Copy link
Member

No description provided.

Copy link
Member

@gitferry gitferry left a comment

Choose a reason for hiding this comment

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

I think this is specific to babylon so we should do the check when we initiate the babylon client, i.e.,

func NewBabylonController(

@KonradStaniec
Copy link

I think this is specific to babylon so we should do the check when we initiate the babylon client, i.e.,

This is good point!

Though one thing is, that functions starting with New.. should be side effect free and querying external service is definitely counted as side effect. So if we want to that at client level we should introduce func Start() error which will perform this initialisation logic.

@Lazar955
Copy link
Member Author

I think this is specific to babylon so we should do the check when we initiate the babylon client, i.e.,

func NewBabylonController(

Make sense. Also agree with @KonradStaniec regarding Start(). Added in most recent commit

@Lazar955 Lazar955 requested a review from gitferry January 27, 2025 10:51
Copy link
Member

@gitferry gitferry left a comment

Choose a reason for hiding this comment

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

Lgtm!

@@ -89,6 +91,8 @@ type ClientController interface {
// the value zero should be returned.
QueryFinalityActivationBlockHeight() (uint64, error)

NodeTxIndexEnabled() (bool, error)
Copy link
Member

Choose a reason for hiding this comment

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

This should be removed

@Lazar955 Lazar955 merged commit 2765ab0 into main Jan 27, 2025
15 checks passed
@Lazar955 Lazar955 deleted the lazar/check_tx_index branch January 27, 2025 14:23
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