-
Notifications
You must be signed in to change notification settings - Fork 156
chore: move all interfaces to dedicated package #1187
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
Open
tmigone
wants to merge
29
commits into
horizon
Choose a base branch
from
tmigone/horizon-interfaces
base: horizon
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
29 commits
Select commit
Hold shift + click to select a range
be8d5a1
chore: move all interfaces to dedicated package
tmigone 942c1aa
chore: lint
tmigone f86ffbb
chore: build interface types
tmigone 63b8c27
fix: import in contracts package
tmigone 32ef00c
chore: move sdk to own repo, update versions
tmigone f8bb3a5
chore: update toolshed to use interfaces package
tmigone 7f21b0b
chore: break circular dep in toolshed, fix contracts ethers versions
tmigone be1acc5
chore: lint
tmigone 8aef58d
chore: fully implement interfaces for toolshed
tmigone 0112d1b
chore: import hre statically in horizon and subgraph service
tmigone 9c67f29
chore: remove unused files
tmigone 495e576
chore: lint packages
tmigone 71b90c4
fix: remove dead import
tmigone 0f7080d
feat: create address book if doesnt exist when running deploy task
tmigone 2bfd7e0
fix: add some missing interface details
tmigone 62eb5aa
fix: couple fixes to make integration tests pass
tmigone 5f707a8
chore: update subgraph service to work wiht latest interfaces
tmigone 2c94da4
chore: final changes to use interfaces package
tmigone 5ef5fe9
chore: lint and replace some imports
tmigone e75ae57
chore: bump package versions and publish
tmigone 039a32e
chore: make interfaces package public
tmigone f6c2e1c
chore: bump package versions
tmigone 31edeed
fix: ensure dist is published for interfaces package
tmigone 4f23eea
chore: add missing interfaces and fix npm publishing issues
tmigone fbe38f9
fix: add ICuration to L2Curation interface
tmigone ed51926
fix: add more stuff missing from interfaces
tmigone b4fe809
chore: ensure workspace dependencies are not pinned to specific versions
tmigone a8469af
chore: better gitignore
tmigone 8b7f14c
chore: fix pnpm lock file
tmigone File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file was deleted.
Oops, something went wrong.
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good for this PR, a discussion point though:
Locally I am trying splitting
build
tobuild:dep
(build dependencies) andbuild:self
(only build package currently in). Then:build
->pnpm build:dep && pnpm build:self
build
->pnpm -r run build:self
, because this one should not be recursive, just do each in individually once in dependency order.test
in (non-root) package atpnpm build &&
to existing. Bascially I don't want to have to worry about rebuilding, although might be worth making original targettest:no-build
or something so can do that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we should keep child packages long term? I don't exactly remember at this point why we added those, I think to break the cyclic dependency? If so, perhaps with
interfaces
package we can simplify it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need child packages, and/or siblings that combine dependencies across contract packages.
They are to break cyclic dependencies, I think interfaces does not by itself fully solve that. It might mean that contract packages no longer rely on toolshed? Or is that still via dynamic loading? However, deployment has cross package needs for compiled contracts. (I think there is an argument for having a single deploy sibling, that spans contract packages.)
The same scenario for tests, similar nuances to deploy, but I think not as strong a case for (just) a single test package spanning contract packages.
If we do not separate them, I think we can appear to get away with it by recompiling dependencies, which the hardhat-dependency-compiler can do, but then you end up with other issues due to that duplication and inconsistency.
I suspect contract compilation should be separate reusable step from what we do with the compilied artifacts (deployment, testing, etc). But I might be missing something; I am new to this package management system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. I'm not too sold on any alternative to be honest, there are things i dislike in both :p. But happy to keep iterating.