-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Update registry assets to simple solidity registry implementation #1400
Update registry assets to simple solidity registry implementation #1400
Conversation
3bc1f79
to
9800a4c
Compare
9800a4c
to
360a55f
Compare
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'm inclined to have you get 1-2 more people to look at this before merging just because it's smart contract code. They don't necessarily have to be solidity experts but more 👀 might catch something I missed.
return releaseId; | ||
} | ||
|
||
function cutRelease( |
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.
maybe name _cutRelease
to signal privateness.
string memory version | ||
) | ||
public | ||
view |
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.
Should this be pure
?
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.
Yes, it should. But the ERC / Interface define this function as view
. Which looks to be the case b/c in the complex solidity implementation, it generates these values via the PackageDB
and ReleaseDB
. So, I left it as view
here to maintain compatibility with the erc/ PackageRegistryInterface
.
uint packageReleaseCount | ||
) | ||
private | ||
pure |
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.
weird indentation
Yup, doing that in ethpm/solidity-registry#1. The contract here is |
What was wrong?
New simplified solidity registry implementation [here] is the default registry implementation moving forward.
ethpm
's assets folder needs updating to use this implementation.Cute Animal Picture