-
Notifications
You must be signed in to change notification settings - Fork 22
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
Making the repo hardhat compatible #68
base: main
Are you sure you want to change the base?
Making the repo hardhat compatible #68
Conversation
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.
Let me know what you think!
"scripts": { | ||
"test": "forge test", | ||
"format": "forge fmt", | ||
"prepublishOnly": "echo 'Error: Please run 'scripts/publish.sh' to publish.' && exit 1" |
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.
Why error here instead of running the script? Presumably because the script is modifying the package.json?
I think the package.json should probably not have any scripts and instead publish should be implemented as a target in the Makefile (so we stick to one method of building / doing things).
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.
This is the same method used by OpenZeppelin and major libraries developers to be sure that you can import files without specifying src
or contracts
. In other words, the package.json
file that is used to publish is a modified version of the primary one, in order to specify the files we want to publish and exclude anything else.
I wrote a publish.sh
script because it was simpler and less invasive. Of course, you guys can put that stuff in your building/deploying process, which I think would be better.
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.
BTW, if you do not block the prePublish, someone can accidentally publish it not in the best way, creating a potential disaster for devs who are using the package.
@@ -0,0 +1,25 @@ | |||
{ | |||
"name": "wormhole-solidity-sdk", |
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.
This package should be published under the @wormhole-foundation scope.
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.
Of course, but since I contacted someone there for a while and nobody cared about it, months ago I published an Hardhat version of it by myself, in order to use it in my projects and to allow a couple of friends who had the same issue to work easily inside hardhat. The beauty of open source is that it allows work-around, even when the official published doesn't give a s**t about something :-)
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.
Yeah, I agree. I apologize about the neglect here, the ecosystem is still in the process of slowly getting its shit together. But I'm now the new Sheriff in town and this sort of neglect will not happen under my watch.
scripts/mocks/HelloWormhole.sol
Outdated
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.
What's the purpose of this example contract? I see that it's being compiled as a test and then deleted before publishing, but it's unclear to me what it really is testing, if the rest of the repo compiles successfully.
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.
In my previous version, even when the contracts where compiling well, during the import it was failing. So, my workaround is to create a temporary contract that imports the SDK, verify that everything compiles correctly, and then remove the no-more-useful temporary contract. When we are sure that there isn't that problem anymore, we can of course skip that. Consider it as a double-check test.
3bf3eff
to
8dae7ff
Compare
Just to keep you updated: I'll soon be picking this up and modify it a bit further, and eventually release an npm package for hardhat consumption under the @wormhole-foundation npm scope. I'd like to hit v1.0 before that though. |
After a chat with @nik-suri I found an hour today to make a simple change to the repo in order to make it publishable to npm for hardhat users.
It works well.
I just published it to Npm at
https://www.npmjs.com/package/wormhole-solidity-sdk
When running
scripts/publish.sh
, the script copiessrc
tocontracts
, modifies the files to remove the remapping, not currently supported by hardhat, and set a newpackage.json
to publish just the contracts' files.