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.

Sign up for GitHub

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

Jump to bottom

fix circular dependencies #676

Closed
wants to merge 3 commits into from
Closed

fix circular dependencies #676

wants to merge 3 commits into from

Conversation

mktcode
Copy link
Contributor

@mktcode mktcode commented Aug 3, 2022 •
edited
Loading

Fixes #544

Changes proposed in this pull request:

  • move voting and validation from utils to top-level/index

In the frontend we import from the src directory directly (e.g. '@snapshot-labs/snapshot.js/src/voting/quadratic') but hub uses snapshot.utils (1, 2). This needs to be adjusted when updating snapshot.js over there. Also regarding forks and other integrations we technically should bump version to v1.0.0 if we want to strictly adhere to semver.

Probably the remaining circular dependencies should be fixed too.

src/utils.ts -> src/utils/multicaller.ts -> src/utils.ts
src/utils.ts -> src/utils/blockfinder.ts -> src/utils.ts
src/utils.ts -> src/sign/utils.ts -> src/sign/eip1271.ts -> src/utils.ts

But since this PR already fixes #544 I don't really want to go down that rabbit hole now. Not this month at least.

bonustrack
bonustrack reviewed Aug 3, 2022
View reviewed changes
package.json
@@ -1,6 +1,6 @@
{
"name": "@snapshot-labs/snapshot.js",
"version": "0.4.13",
"version": "1.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

Can we keep v1 for a big refactoring?

@bonustrack
Copy link
Member

I think we need the validations in snapshot-strategies repo, snapshot.js should only contain code from us

@mktcode
Copy link
Contributor Author

mktcode commented Aug 4, 2022 •
edited
Loading

Yep. Couldn't agree more. Although in regards to #544 it's the voting causing the problem. I would then maybe start by adding the validations to the strategy repo, update frontend and hub, and then remove validations here and merge the PR.

Can we keep v1 for a big refactoring?

Not sure what a big refactoring would exactly mean. But in terms of semantic versioning it doesn't really matter. It's a convention to exactly not have such discussion actually. If the update contains a breaking change -> major version update. If it only contains new, non-breaking features -> minor version update. Bugfixes and code cleanup -> patch version update. The only problem with semver is that there's no guarantee that it's strictly followed. But I don't want to contribute to that problem.

And I'd like to add to this that semver does not necessarily have something to do with what versions or updates or whatever is communicated publicly. It's a pure technical purpose. Snapshot.js 2 could internally be v17.3.67.

@mktcode mktcode mentioned this pull request Aug 4, 2022
Merged
@ChaituVR ChaituVR removed their request for review July 20, 2023 19:14
@mktcode mktcode closed this Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bonustrack bonustrack bonustrack left review comments

@samuveth samuveth Awaiting requested review from samuveth

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Circular dependencies
2 participants
@mktcode @bonustrack