Skip to content

Add context.Context to methods defined in chain, prover, and signer module interfaces #153

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

Merged
merged 6 commits into from
Feb 19, 2025

Conversation

abicky
Copy link
Contributor

@abicky abicky commented Feb 12, 2025

Description

This PR is the first step to integrate with OpenTelemetry to improve the observability of yui-relayer.
I will divide the whole integration process into the following phases:

  1. Add a context.Context argument to the methods defined in chain module interfaces, prover module interfaces, and signer module interfaces
  2. Fix method signatures of the modules in other repositories
  3. Add a context.Context argument to appropriate methods that are unlikely to be used from other repositories
  4. Replace context.TODO() with an appropriate context
  5. Integrate with OpenTelemetry

This PR includes the changes in the first phase and I have added a context.Context argument to the methods likely to access external services or create a goroutine, and context.TODO() to the method calls in a mechanical manner.
Although the main purpose is to improve the observability, I think it is common to add a context.Context argument to such methods, whose process we need to cancel in some occasions.

Note that I don't add a context.Context argument to the methods that are likely to be a mere getter.

Considerations

This PR includes breaking changes, but they are acceptable according to Module version numbering:

Signals that the module is still in development and unstable. This release carries no backward compatibility or stability guarantees.

Note

I have examined the following interfaces:

% git grep -E 'type \w+ interface \{' ':!chains'
config/module.go:9:type ModuleI interface {
core/chain.go:59:type Chain interface {
core/chain.go:96:type ChainInfo interface {
core/chain.go:114:type MsgEventListener interface {
core/chain.go:120:type IBCQuerier interface {
core/chain.go:127:type ICS02Querier interface {
core/chain.go:137:type ICS03Querier interface {
core/chain.go:143:type ICS04Querier interface {
core/chain.go:173:type ICS20Querier interface {
core/chain.go:181:type LightClientICS04Querier interface {
core/chain.go:187:type QueryContext interface {
core/config.go:28:type ConfigI interface {
core/config.go:50:type ChainConfig interface {
core/config.go:57:type ProverConfig interface {
core/headers.go:12:type Header interface {
core/headers.go:19:type SyncHeaders interface {
core/headers.go:37:type ChainInfoLightClient interface {
core/headers.go:43:type ChainLightClient interface {
core/msg.go:11:type MsgID interface {
core/msg.go:17:type MsgResult interface {
core/msg.go:30:type MsgEventLog interface {
core/provers.go:13:type Prover interface {
core/provers.go:28:type StateProver interface {
core/provers.go:38:type LightClient interface {
core/provers.go:57:type FinalityAware interface {
core/provers.go:64:type FinalityAwareChain interface {
core/provers.go:70:type ChainInfoICS02Querier interface {
core/strategies.go:9:type StrategyI interface {
metrics/metrics.go:30:type ExporterConfig interface {
signer/signer.go:9:type SignerConfig interface {

And here is my note on whether to add a context argument:

  • Chain
    • This interface embeds ChainInfo, IBCQuerier, and ICS20Querier
    • Example implementations:
      • github.com/datachainlab/ethereum-ibc-relay-chain/pkg/relay/ethereum.Chain
      • github.com/hyperledger-labs/yui-relayer/chains/tendermint.Chain
    • All the methods of IBCQuerier and ICS20Querier have QueryContext containing context.Context as the first argument, so we don't need to change the signature
    • Other methods except for SendMsgs, GetMsgResult, LatestHeight, and Timestamp are expected to be simple getters, setters, or fixed values, so we don't need a context
  • MsgEventListener
    • I could not find any implementations but some modules may access external services, so I have added a context.Context argument
  • ConfigI
    • The method UpdatePahtConfig is unlikely to access external services or create a goroutine, so we don't need to change the signature
  • ChainConfig
    • The methods Build and Validate are likely to be so simple that we don't need a context
  • ProverConfig
    • Same as ChainConfig
  • SyncHeaders
    • This interface is used internally, so I won't change the signatures for now, and I will change them in another PR
  • MsgID
    • Is_MsgID is just used for an annotation to indicate that the struct is a kind of MsgID
  • MsgResult
    • The methods are usually mere getters of struct fields
  • MsgEventLog
    • is_MsgEventLog is just used for an annotation to indicate that the struct is a kind of MsgEventLog
  • Prover
    • Almost all the methods usually access blockchain nodes, so I have added a context.Context argument
  • StrategyI
    • Same as SyncHeaders
  • ExporterConfig
    • This interface is used internally
  • SignerConfig
    • Same as ChainConfig
  • Signer

@abicky abicky marked this pull request as ready for review February 13, 2025 00:24
@abicky abicky requested a review from a team as a code owner February 13, 2025 00:24
@abicky
Copy link
Contributor Author

abicky commented Feb 13, 2025

@bluele @siburu Can you take a look?

@siburu
Copy link
Contributor

siburu commented Feb 13, 2025

@abicky Thank you for your careful consideration. Basically OK, but I noticed that Chain.Timestamp and Prover.CreateInitialLightClientState take both context.Context and ibcexported.Height as arguments. Therefore wouldn't it be more natural for these functions to accept a single QueryContext argument instead?

@abicky
Copy link
Contributor Author

abicky commented Feb 14, 2025

@siburu Thank you for your feedback!
My understanding is that QueryContext is used for an option when a client sends a request to a blockchain node as follows:

If I'm correct, it is not appropriate to use QueryContext for Timestamp and CreateInitialLightClientState because the height is an argument of the method, not an option for a RPC request.

Here are also some example methods that take both of a context and a height as their arguments:

  • core.StateProver.ProveHostConsensusState
  • core.ICS02Querier.QueryClientConsensusState

Copy link
Contributor

@siburu siburu left a comment

Choose a reason for hiding this comment

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

Sorry for the late review. LGTM. Thank you!

@bluele bluele self-requested a review February 18, 2025 10:52
Copy link
Member

@bluele bluele left a comment

Choose a reason for hiding this comment

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

LGTM👍

Copy link

@mizuochik mizuochik left a comment

Choose a reason for hiding this comment

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

LGTM!

Although SyncHeaders and StrategyI support for contexts is important for trace implementation, I understand it's a different PR.

@siburu siburu merged commit 5962607 into hyperledger-labs:main Feb 19, 2025
7 checks passed
abicky added a commit to datachainlab/ibc-hd-signer that referenced this pull request Feb 19, 2025
@abicky abicky deleted the add-context branch February 19, 2025 10:14
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.

4 participants