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

Move chain util to libs #6217

Merged
merged 8 commits into from
Jan 5, 2024
Merged

Conversation

Rotorsoft
Copy link
Contributor

@Rotorsoft Rotorsoft commented Jan 4, 2024

Starts moving utilities from common-common to libs, in this case chain related code under eth and cosmos-ts

Link to Issue

Closes: #6208

Description of Changes

  • Moved folders to libs/chains
  • Fixed imports to use @hicommonwealth/chains
  • libs/chains/src/index.ts is the public interface exporting types and utils

Test Plan

  • @timolegros what is the best way to test this?
  • Testing on frack
  • All files inside the lib are just a copy from common-common, no need to review much there!
  • Review the imports and any potential side effects

@Rotorsoft Rotorsoft requested review from timolegros and mhagel January 4, 2024 21:14
@Rotorsoft Rotorsoft temporarily deployed to commonwealth-frack January 5, 2024 13:48 Inactive
@Rotorsoft Rotorsoft temporarily deployed to commonwealth-frack January 5, 2024 14:03 Inactive
@Rotorsoft Rotorsoft requested a review from lzach83 January 5, 2024 14:58
@timolegros
Copy link
Collaborator

timolegros commented Jan 5, 2024

We can test EVM/ETH side by viewing communities that have associated contracts like dydx. For the cosmos side, we should use the Heroku csdk chains to create/interact with a proposal.

@kurtisassad could you check this to make sure there are no changes to the Webpack bundle size (i.e. tree shaking isn't affected)? Not sure if that's a possible issue here but since we are importing some of these files on the client and we are moving a significant number of files I want to make sure.

@Rotorsoft
Copy link
Contributor Author

We can test EVM/ETH side by viewing communities that have associated contracts like dydx. For the cosmos side, we should use the Heroku csdk chains to create/interact with a proposal.

@timolegros who can help me walk through these?

@timolegros
Copy link
Collaborator

We can test EVM/ETH side by viewing communities that have associated contracts like dydx. For the cosmos side, we should use the Heroku csdk chains to create/interact with a proposal.

@timolegros who can help me walk through these?

Eth side:

  • Just visit dydx community and make sure console isn't logging a bunch of errors
  • Load proposals page and same thing make sure console isn't logging a bunch of errors

Cosmos side:

  • Mark set this up so I'm not super familiar but we should be able to follow the docs he has on it.
  • Will need to deploy to Frick though I believe

I will run through both of these myself to review anyways so we can just hop in huddle when I do that.

@Rotorsoft Rotorsoft temporarily deployed to commonwealth-frick January 5, 2024 20:43 Inactive
@CowMuon CowMuon self-requested a review January 5, 2024 21:37
Copy link
Contributor

@CowMuon CowMuon left a comment

Choose a reason for hiding this comment

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

Regardless the high surface area (346 files changed) here, this is looking fine to merge.
Having said that, do we need this in 0.9.0? @Rotorsoft @ilijabojanovic My preference would be for Mark to take another review pass on the Cosmos side of things to make sure nothing is missed.

@Rotorsoft
Copy link
Contributor Author

Regardless the high surface area (346 files changed) here, this is looking fine to merge. Having said that, do we need this in 0.9.0? @Rotorsoft @ilijabojanovic My preference would be for Mark to take another review pass on the Cosmos side of things to make sure nothing is missed.

@timolegros and I just tested cosmos on frick and everything looked fine

@Rotorsoft Rotorsoft merged commit 9267996 into master Jan 5, 2024
7 checks passed
@Rotorsoft Rotorsoft deleted the rotorsoft/6208.move-chain-util-to-libs branch January 5, 2024 21:44
@CowMuon
Copy link
Contributor

CowMuon commented Jan 5, 2024

Well if the tests pass and we are confident in our coverage, Mark can pro forma sign off when he's back on Monday (and we'll plan on shipping in 0.9.0)

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.

Move eth and cosmos-ts utilities to libs/chains
3 participants