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

feat: Client implementation #1888

Merged
merged 23 commits into from
Apr 6, 2023
Merged

feat: Client implementation #1888

merged 23 commits into from
Apr 6, 2023

Conversation

robert-zaremba
Copy link
Member

Description

Closes: #1878

Concept:

  • have a common client which can be used in e2e tests and external tools
  • provide separate modules:
    • first providing SDK client methods, and
    • second: providing umee specific methods
  • the first package is a separate Go module and can be imported by other chains.

@robert-zaremba robert-zaremba requested review from a team as code owners March 5, 2023 01:56
github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 5, 2023
View reviewed changes
client/oracle.go
defer cancel()

queryResponse, err := c.OracleQueryClient().Params(ctx, &oracletypes.QueryParams{})
return queryResponse.Params, err

Check warning

Code scanning / CodeQL

Missing error check

[queryResponse](1) may be nil at this dereference because [err](2) may not have been checked.
client/oracle.go
defer cancel()

queryResponse, err := c.OracleQueryClient().ExchangeRates(ctx, &oracletypes.QueryExchangeRates{})
return queryResponse.ExchangeRates, err

Check warning

Code scanning / CodeQL

Missing error check

[queryResponse](1) may be nil at this dereference because [err](2) may not have been checked.
client/oracle.go
defer cancel()

resp, err := c.OracleQueryClient().Medians(ctx, &oracletypes.QueryMedians{})
return resp.Medians, err

Check warning

Code scanning / CodeQL

Missing error check

[resp](1) may be nil at this dereference because [err](2) may not have been checked.
client/oracle.go
defer cancel()

queryResponse, err := c.OracleQueryClient().MedianDeviations(ctx, &oracletypes.QueryMedianDeviations{})
return queryResponse.MedianDeviations, err

Check warning

Code scanning / CodeQL

Missing error check

[queryResponse](1) may be nil at this dereference because [err](2) may not have been checked.
sdkclient/chain_height.go
HeightChanged: make(chan int64),
}

go chainHeight.subscribe(ctx, rpcClient, newBlockHeaderSubscription)

Check notice

Code scanning / CodeQL

Spawning a Go routine

Spawning a Go routine may be a possible source of non-determinism
@robert-zaremba robert-zaremba requested a review from a team as a code owner March 5, 2023 22:53
@toteki
Copy link
Member

toteki commented Mar 6, 2023

Should fix those missing error checks - they'll lead to panics on (for example) resp.Medians when resp == nil

@robert-zaremba
Copy link
Member Author

Yes, will look at this nil checks tomorrow. However this should not block the review.

@robert-zaremba
Copy link
Member Author

up, waiting for review

zarazan
zarazan reviewed Mar 10, 2023
View reviewed changes
Copy link
Contributor

@zarazan zarazan left a comment

Choose a reason for hiding this comment

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

I like what you did with cleaning up the encoding and separating the two Clients. Just a couple naming nit picks. I look forward to pulling this into its own package!

client/client.go Outdated Show resolved Hide resolved
client/client.go Show resolved Hide resolved
sdkclient/client.go Outdated Show resolved Hide resolved
client/client.go Show resolved Hide resolved
client/client.go Outdated Show resolved Hide resolved
@zarazan zarazan mentioned this pull request Mar 10, 2023
Open
@robert-zaremba robert-zaremba added this pull request to the merge queue Apr 6, 2023
Merged via the queue into main with commit b0f1cfb Apr 6, 2023
@robert-zaremba robert-zaremba deleted the robert/client branch April 6, 2023 14:02
This was referenced Apr 19, 2024
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zarazan zarazan zarazan approved these changes

Assignees
No one assigned
Labels
C:API
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Move client code to a new Go module
4 participants
@robert-zaremba @toteki @zarazan @gsk967