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/Updated Superchain DEX projects #643

Open
wants to merge 49 commits into
base: main
Choose a base branch
from

Conversation

kaiblade
Copy link

No description provided.

Copy link
Member

@ccerv1 ccerv1 left a comment

Choose a reason for hiding this comment

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

The entry looks great! We haven't tested anything through Soneium yet.
Is there are a public GitHub URL that we can associate with the project? That would be the only thing missing typically.

@kaiblade
Copy link
Author

The entry looks great! We haven't tested anything through Soneium yet. Is there are a public GitHub URL that we can associate with the project? That would be the only thing missing typically.

Yes. Just added the Github link. Thanks for checking this.

Copy link
Member

@ccerv1 ccerv1 left a comment

Choose a reason for hiding this comment

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

Promote the Github artifact to a first level element

social:
twitter:
- url: https://twitter.com/kyofinance
github:
Copy link
Member

Choose a reason for hiding this comment

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

This is a first level entry, at the same level as websites, social, etc.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@ccerv1
Copy link
Member

ccerv1 commented Feb 14, 2025

@kaiblade do you want to convert this from draft mode now so I can review / merge it?

@kaiblade
Copy link
Author

kaiblade commented Feb 14, 2025

@kaiblade do you want to convert this from draft mode now so I can review / merge it?

@ccerv1 Not yet, still have quite a bit of work to do. I'll let you know once I'm ready. Thanks for asking.

@kaiblade kaiblade marked this pull request as ready for review February 22, 2025 10:32
@kaiblade kaiblade requested a review from ccerv1 February 22, 2025 10:40
@kaiblade
Copy link
Author

@ccerv1 Hello, kindly review this PR.

@kaiblade
Copy link
Author

/validate 23201ce

Copy link
Member

@ccerv1 ccerv1 left a comment

Choose a reason for hiding this comment

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

lgtm! great work

@ccerv1
Copy link
Member

ccerv1 commented Feb 24, 2025

/validate 23201ce

Copy link
Member

@ccerv1 ccerv1 left a comment

Choose a reason for hiding this comment

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

There are several issues that need to be addressed:

1.name fields take strings, not arrays

For example, this will be rejected

    name:
      - FarmingVault_cbBTC

Should be name: FarmingVault_cbBTC

  1. celo is not a supported network. Please remove this from all networks. Any contracts only on Celo should also be removed.

  2. token is not a supported address tag

I believe these are the main issues.

Please try developing locally and running pnpm validate to check for schema correctness.

@ccerv1
Copy link
Member

ccerv1 commented Feb 24, 2025

I was unable to make commits to your branch, so I pushed them to this PR. I think I reviewed about half of the projects.
#664

@ccerv1 ccerv1 added the pr:critical_issue PR has critical issue preventing approval label Feb 24, 2025
@kaiblade
Copy link
Author

/validate 9f2ecac

@kaiblade kaiblade requested a review from ccerv1 February 24, 2025 15:12
@kaiblade kaiblade deployed to external-prs-app February 25, 2025 10:34 — with GitHub Actions Active
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:critical_issue PR has critical issue preventing approval
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants