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

add new repo structure #2

Merged
merged 12 commits into from
Feb 20, 2024
Merged

Conversation

ribeirojose
Copy link

@ribeirojose ribeirojose commented Feb 20, 2024

closes #3

Copy link

linear bot commented Feb 20, 2024

@ribeirojose ribeirojose force-pushed the jose/cowpy-2-setup-base-repo-structure branch 4 times, most recently from 5b60086 to 9b852ce Compare February 20, 2024 15:08
@ribeirojose ribeirojose force-pushed the jose/cowpy-2-setup-base-repo-structure branch from 9b852ce to bfc594f Compare February 20, 2024 15:20
@ribeirojose ribeirojose marked this pull request as ready for review February 20, 2024 15:33
@ribeirojose ribeirojose force-pushed the jose/cowpy-2-setup-base-repo-structure branch from f2859b7 to 6af0426 Compare February 20, 2024 15:52
@ribeirojose ribeirojose force-pushed the jose/cowpy-2-setup-base-repo-structure branch from e3dd4e7 to e21122d Compare February 20, 2024 16:02
@ribeirojose ribeirojose force-pushed the jose/cowpy-2-setup-base-repo-structure branch from 06c9abf to 89c7114 Compare February 20, 2024 16:27
Copy link

@yvesfracari yvesfracari left a comment

Choose a reason for hiding this comment

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

lgtm!

@@ -0,0 +1,4 @@
from cow_py.core import constants, cow_error, abis, chains, config

Choose a reason for hiding this comment

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

would use the same name of the ts package (common)

Comment on lines 25 to 29
CHAIN_SCANNER_MAP = {
Chain.MAINNET: "https://etherscan.io",
Chain.GNOSIS: "https://gnosisscan.io",
Chain.SEPOLIA: "https://sepolia.etherscan.io/",
}

Choose a reason for hiding this comment

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

i don't think that we will need this

Copy link
Author

Choose a reason for hiding this comment

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

I'll leave it here for now though

@@ -1,4 +1,5 @@
# cow-py

Choose a reason for hiding this comment

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

This was made by the formatter?

Copy link

Choose a reason for hiding this comment

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

FYI, on this, I think we would at some stage want to change the upstream cow-sdk to have contracts as a module within, so you could have:

  • contracts module
  • within the contracts module, the abi.

So all consumers of the cow-py don't have to worry about ABI encoding etc (ie. shouldn't have to touch it at all, and the SDK can do it for them).

I won't hold up the merge on this, and happy for you to roll forward 🛼

Copy link
Author

Choose a reason for hiding this comment

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

agreed! Just sent a commit with those changes 😄

@ribeirojose ribeirojose merged commit 153551c into main Feb 20, 2024
5 checks passed
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.

[COWPY-2] Setup base repo structure
4 participants