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

Added cw-orch + Implemented Deploy trait #54

Merged
merged 26 commits into from
Jan 8, 2024

Conversation

Kayanski
Copy link
Contributor

This PR aims at adding cw-orch into the repository.

It is done in 3 steps :

  1. Create an interface package with interface definitions for each main contract
  2. Implement the Deploy trait to easily deploy and reuse polytone on the chain where it is deployed
  3. Add macros on the ExecuteMsg and QueryMsg definitions of voice/proxy/note

Copy link
Member

Choose a reason for hiding this comment

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

Do we have to commit the wasm binaries?

Copy link
Member

Choose a reason for hiding this comment

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

Could we have a cargo command or something that builds them and then runs cw-orch?

Copy link
Contributor Author

@Kayanski Kayanski Oct 19, 2023

Choose a reason for hiding this comment

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

The idea is for projects that integrate with polytone to be able to import the cw-orch structure along with the artifacts and the deployments (code_ids here).
Compiling the binaries from an imported project yourself, doesn't really make sense and adds a level of complexity for contract developers that can be avoided by having the artifacts in the repo.
What do you think ?

Copy link
Member

Choose a reason for hiding this comment

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

Binaries can be compiled and added to tagged releases. A simple script can fetch them.

Alternatively, people can compile them.

Just worry about them:
a.) getting out of date
b.) having to check them every PR
c.) taking up space in git history (not too big of a deal, but minor annoyance IMO)

Copy link

@CyberHoward CyberHoward Oct 20, 2023

Choose a reason for hiding this comment

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

We have a github action that builds and commits them whenever something is committed to main.

We could move that over and keep the artifacts dir in the .gitignore. Let the CI handle building them.

image

Copy link
Member

Choose a reason for hiding this comment

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

We could move that over and keep the artifacts dir in the .gitignore. Let the CI handle building them.

Sounds sensible to me!

@CyberHoward
Copy link

@JakeHartnell Could you use your credentials in the commit step of the artifacts build?

I tested the action here.

@@ -0,0 +1,19 @@
[package]
name = "cw-orch-polytone"
Copy link
Member

Choose a reason for hiding this comment

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

Nit about the folder name... interface implies to me that it's an interface (i.e. the messages, responses, and general structs used by the polytone contracts. However, cw-orch-polytone is not that.

IMO, let's call the folder cw-orch-polytone or deployer or something like that.

A question for you for other libraries you might add something similar to (DAO DAO would be sick)... is this a package? Or perhaps it belongs in another folder.

Keeping it in packages is fine for now, but please rename to cw-orch-polytone or deployer (slight preference for the former). Sorry in advance for this annoying nit pick. 😂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will change the name, but that's a package ! That's something you can import from other projects to use the interface and associated state/artifacts.

state.json Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Is state the standard filename for cw-orch? IMO, something less generic would be nicer for projects integrating cw-orch. Maybe something like cw-orch-state.json? Says exactly what it is... so I don't have to wonder... what is this random state.json file.

Copy link
Member

Choose a reason for hiding this comment

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

Also, maybe double check if new chains have been added here?

.gitignore Outdated
hash.txt
contracts.txt
artifacts/
Copy link
Member

Choose a reason for hiding this comment

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

Let's re-add artifacts to the .gitignore here. As discussed with @CyberHoward.

@JakeHartnell
Copy link
Member

FYI, seems like CI keeps failing... maybe unrelated, but would like to see CI pass before merging: https://github.com/DA0-DA0/polytone/actions/runs/6507738068

This is getting close!

@CyberHoward
Copy link

FYI, seems like CI keeps failing... maybe unrelated, but would like to see CI pass before merging: https://github.com/DA0-DA0/polytone/actions/runs/6507738068

This is getting close!

This CI job is on main (https://github.com/DA0-DA0/polytone/blob/main/.github/workflows/audit_dependencies.yml) so not something we touched. Should be fixed in a different PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think about including the addresses of a deployment just like I did here, and adding those you already have done on other chains in the cw-orch-state.json file as well ?
This would help projects identify the deployed polytone addresses to not have to worry about them, just using the polytone package to integrated with ?

Copy link
Member

Choose a reason for hiding this comment

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

In favor.

@JakeHartnell
Copy link
Member

Please rebase or merge latest main, CI has been fixed: #56

@Kayanski
Copy link
Contributor Author

Kayanski commented Dec 7, 2023

Please rebase or merge latest main, CI has been fixed: #56

Rebased now

Copy link
Member

@NoahSaso NoahSaso 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! just remove the artifacts folder and we'll be good to go :)

@Kayanski
Copy link
Contributor Author

Kayanski commented Jan 8, 2024

looks good! just remove the artifacts folder and we'll be good to go :)

This is now done

@NoahSaso NoahSaso merged commit 33bd9ab into DA0-DA0:main Jan 8, 2024
@Kayanski Kayanski deleted the add-cw-orch branch January 19, 2024 08:17
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