Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

misc(state/core_accessor): add retry logic for creating a txClient #4053

Merged
merged 4 commits into from
Jan 23, 2025

Conversation

vgonkivs
Copy link
Member

Create a txClient in runtime if it was initialized during the startup

@vgonkivs vgonkivs added the kind:misc Attached to miscellaneous PRs label Jan 20, 2025
@vgonkivs vgonkivs self-assigned this Jan 20, 2025
@codecov-commenter
Copy link

codecov-commenter commented Jan 20, 2025

Codecov Report

Attention: Patch coverage is 52.38095% with 20 lines in your changes missing coverage. Please review.

Project coverage is 44.79%. Comparing base (2469e7a) to head (a2b1919).
Report is 428 commits behind head on main.

Files with missing lines Patch % Lines
state/core_access.go 52.38% 14 Missing and 6 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4053      +/-   ##
==========================================
- Coverage   44.83%   44.79%   -0.05%     
==========================================
  Files         265      308      +43     
  Lines       14620    22456    +7836     
==========================================
+ Hits         6555    10059    +3504     
- Misses       7313    11312    +3999     
- Partials      752     1085     +333     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

why not just try to set it up in getSigner if it's == nil ?

@vgonkivs
Copy link
Member Author

vgonkivs commented Jan 20, 2025

getSigner parses the TxConfig(signer is not required to be a defaultAccount) which is not the same as creating a client. I actually haven't thought about doing it there. If you have a strong opinion about it, I can definetely create it there.

@renaynay
Copy link
Member

My point is getSigner is called everywhere where the signer is needed, so why can't we lazy re-init in there if signer == nil ?

@vgonkivs
Copy link
Member Author

vgonkivs commented Jan 20, 2025

signer != txClient. Signer is a property of TxConfig in that context and it can be nil: this means that defaultAccount will sign the transaction. txClient is used in two places: submitPayForBlob(for blob submission) and submitMsg(for all other Write transactions).

@renaynay
Copy link
Member

@vgonkivs I mean why can't we have the same logic (ca.getTxClient()) to either return already-initialised client or try to lazy re-init the client?

so instead of on #L122 instead of calling setupTxClient, there's a fucntion

getTxClient() which checks if ca.client !=nil, returns the client, but if nil, then it tries to set it up.

@vgonkivs
Copy link
Member Author

Ah, ok. I did not get your initial idea. Sorry. will rework it.

Copy link
Contributor

@cristaloleg cristaloleg left a comment

Choose a reason for hiding this comment

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

SGTM

@renaynay renaynay enabled auto-merge (squash) January 23, 2025 11:04
@renaynay renaynay merged commit 72ef0ac into celestiaorg:main Jan 23, 2025
22 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
kind:misc Attached to miscellaneous PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants