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: 83 add cosmos service: add query and tx clients #96

Merged
merged 28 commits into from
Jul 25, 2022

Conversation

dib542
Copy link
Collaborator

@dib542 dib542 commented Jul 22, 2022

This PR adds an autogenerated queryClient and a semi-autogenerated txClient with generated types from the Cosmos server, for easier using or queries and msgs from within components.

dib542 added 24 commits July 23, 2022 05:15
    - make all cosm.js dependencies 0.27.1
    - no we only have 166 webpack warnings but no errors
    - @cosmjs dependencies and the cosmjs-types dependency do not publish source code, so attempting to read the source code to generate maps will cause warnings
    - other ways to deal with this:
        - have a postinstall script to clone git source libraries into their needed destinations for source map generation to work correctly
        - ignore specifically these errors in Webpack: "Failed to parse source map" & "Error: ENOENT: no such file or directory"
            - this is difficult to do in create-react-app so perhaps it would be better to switch to a create-next-app or Vite build system
@dib542 dib542 requested a review from nickzoum July 22, 2022 20:18
dib542 added a commit that referenced this pull request Jul 22, 2022
@dib542 dib542 linked an issue Jul 22, 2022 that may be closed by this pull request
Copy link
Contributor

@nickzoum nickzoum left a comment

Choose a reason for hiding this comment

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

Looks good, I thought that the requests would be part of this commit (fetching the ticks, etc.)

Comment on lines 62 to 70
interface QueryClientOptions {
addr?: string;
}

const queryClient = async ({
addr = REACT_APP__REST_API,
}: QueryClientOptions = {}) => {
return new Api({ baseUrl: addr });
};
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels a bit too complicated
If we don't add any more attributes something like would be a lot simpler

Suggested change
interface QueryClientOptions {
addr?: string;
}
const queryClient = async ({
addr = REACT_APP__REST_API,
}: QueryClientOptions = {}) => {
return new Api({ baseUrl: addr });
};
const queryClient = (addr = REACT_APP__REST_API ) => {
return new Api({ baseUrl: addr });
}

If we do, I think somewhat spreading the defaults would make a more readable (maybe a tiny bit)

Suggested change
interface QueryClientOptions {
addr?: string;
}
const queryClient = async ({
addr = REACT_APP__REST_API,
}: QueryClientOptions = {}) => {
return new Api({ baseUrl: addr });
};
const defaultOptions = {
addr: REACT_APP__REST_API
};
const queryClient = (options = defaultOptions) => {
const { addr } = { ...defaultOptions, ...options };
return new Api({ baseUrl: addr });
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Resolved by pulling in the already generated ApiConfig type with the baseUrl already predefined in d1729a5

dib542 added a commit that referenced this pull request Jul 22, 2022
dib542 added a commit that referenced this pull request Jul 25, 2022
Copy link
Contributor

@nickzoum nickzoum left a comment

Choose a reason for hiding this comment

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

Looks good, you might be able to simplify queryClient

Comment on lines 65 to 71
const queryClient = async ({
baseUrl = REACT_APP__REST_API,
baseApiParams,
securityWorker,
}: ApiConfig = {}) => {
return new Api({ baseUrl, baseApiParams, securityWorker });
};
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't exactly the same, but I think you could just pass everything else as is, without having to care about the number of arguments being passed in

Suggested change
const queryClient = async ({
baseUrl = REACT_APP__REST_API,
baseApiParams,
securityWorker,
}: ApiConfig = {}) => {
return new Api({ baseUrl, baseApiParams, securityWorker });
};
const queryClient = async (config: ApiConfig = {}) => {
return new Api({ ...config, baseUrl: config?.baseUrl ?? REACT_APP__REST_API });
};

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good point, added a further simplification in 7f08a17

@dib542 dib542 force-pushed the 83-add-query-and-tx-clients branch from f5f9b12 to 7f08a17 Compare July 25, 2022 23:26
@dib542 dib542 merged commit abafc31 into main Jul 25, 2022
@dib542 dib542 deleted the 83-add-query-and-tx-clients branch July 25, 2022 23:35
dib542 pushed a commit that referenced this pull request Jul 25, 2022
## [0.1.8](v0.1.7...v0.1.8) (2022-07-25)

### Features

* 83 add cosmos service: add query and tx clients ([#96](#96)) ([abafc31](abafc31))
@dib542
Copy link
Collaborator Author

dib542 commented Jul 25, 2022

🎉 This PR is included in version 0.1.8 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Cosmos service using cosm.js
2 participants