-
Notifications
You must be signed in to change notification settings - Fork 90
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
Diamond Helper Functions #158
base: master
Are you sure you want to change the base?
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.
Awesome initiative! Eventually, these utility functions will prove very useful to third parties.
@NouDaimon thanks for the feedback! I have a bunch of changes on my local which I plan on pushing this week. I will update the description with my checklist of things before this is ready to come out of draft. |
Any recommendations on where I should add tests and contracts? |
15ce376
to
a7edda4
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.
Still a bit unsure about how to organize this, but definitely a good start.
@0xCourtney Can you list all the places where you think a developer would use the print function? As mentioned in an inline comment, in my opinion that function would fit best in a CLI package. How to print something is fundamentally more opinionated than SS is trying to be. But you might have some use cases in mind that would justify including it here.
I'll put some more thought in to a possible Hardhat plugin.
lib/diamonds.ts
Outdated
} | ||
|
||
return groupFacetCuts(facetCuts); | ||
} |
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.
The add/replace/remove function names imply that a transaction is sent, but really they just generate the FacetCut
formatting. Should change the name to something like generateFacetCutsForAdd
. Can you think of something 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.
These functions are rather specific, so I think they should throw errors if certain passed data can't be added/removed/replaced.
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'd also like a function that takes a list of selectors and targets, and calculates the FacetCut
diff needed to migrate an existing diamond to the given schema.
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.
The add/replace/remove function names imply that a transaction is sent, but really they just generate the
FacetCut
formatting. Should change the name to something likegenerateFacetCutsForAdd
. Can you think of something better?
I asked ChatGPT for help here, how about addUnregisteredSelectorsToFacetCut
? Likewise, the other functions could be renamed replaceRegisteredSelectorsInFacetCut
and removeRegisteredSelectorsFromFacetCut
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'd also like a function that takes a list of selectors and targets, and calculates the
FacetCut
diff needed to migrate an existing diamond to the given schema.
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.
These functions are rather specific, so I think they should throw errors if certain passed data can't be added/removed/replaced.
This reverts commit 9c37405629b7321e0725de8bf30de1e51eb5f685.
In 3d9d456 I introduced the concept of filters. A filter consists of an object containing a contract address and an array of selectors. interface FacetFilter {
contract: string;
selectors: string[];
} The await d.addUnregisteredSelectors(Diamond, [Test1Facet1])
// [{
// target: '0x9fE46736679d2D9a65F0992F2272dE9f3c7fa6e0',
// action: 0,
// selectors: [ '0xc2985578', '0x0716c2ae' ]
// }]
await d.addUnregisteredSelectors(
Diamond,
[Test1Facet1],
[{ contract: Test1Facet1.address, selectors: ['0xc2985578'] }],
[],
);
// [{
// target: '0x9fE46736679d2D9a65F0992F2272dE9f3c7fa6e0',
// action: 0,
// selectors: [ '0xc2985578' ]
// }]
await d.addUnregisteredSelectors(
Diamond,
[Test1Facet1],
[],
[{ contract: Test1Facet1.address,selectors: ['0xc2985578'] }],
)
// [{
// target: '0x9fE46736679d2D9a65F0992F2272dE9f3c7fa6e0',
// action: 0,
// selectors: [ '0x0716c2ae' ]
// }] The |
…tate-solidity into diamond-helper
…xport Use `FacetCutAction` enum in tests
lib/diamond/filters.ts
Outdated
export interface FacetFilter { | ||
contract: string; | ||
selectors: string[]; | ||
} |
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.
It looks like these FacetFilter
objects are used in sets of 3 arrays, corresponding to the FacetCutAction
types. Would it not be better to include the action in the FacetFilter
type?
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 can certainly look into this, I was already planning to replace the only
and except
parameters with filter
in the add/replace/remove API, then add a type
field to FacetFilter
. The new FacetFilter
could look something like this:
export interface FacetFilter {
type: string; // "only" or "except"
action: FacetCutAction;
contract: string;
selectors: string[];
}
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.
lib/diamond/utils.ts
Outdated
target: string = AddressZero, | ||
data: string = '0x', | ||
) { | ||
(await diamond.diamondCut(facetCut, target, data)).wait(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.
How exactly does wait
work?
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.
It may be good to return the TransactionReceipt here. Otherwise, a user may not be able to detect errors or access the receipt, if needed.
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.
lib/diamond/utils.ts
Outdated
return cuts; | ||
} | ||
|
||
export function printFacetCuts( |
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 doesn't seem to print anything.
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 getFacetCut
is more appropriate, the function only structures the FacetCut object.
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.
} | ||
|
||
if (!selectorsReplaced) { | ||
throw new Error('No selectors were replaced in FacetCut'); |
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.
The errors are thrown if all changes are skipped, but I think we'll want to throw if any change is skipped. Need to think about this though.
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.
Would this be the result of using filters or something else?
Co-authored-by: Nick Barry <itsnickbarry@protonmail.ch>
Co-authored-by: Nick Barry <itsnickbarry@protonmail.ch>
Co-authored-by: Nick Barry <itsnickbarry@protonmail.ch>
Hey, anything happening with this PR? Does anyone need some help getting it finished? I saw a lot of promise in it. |
@jhubbardsf I've shifted focus to a WIP plugin that manages diamond upgrades. It's easier to add the functions I need there first, and then determine how to structure what's been added here. This will be merged eventually. |
Summary
EIP2535 is a standard quickly growing in popularity and adoption. While Diamonds offer a great pathway for scaling contracts there are not many tools for managing deployments or upgrades. This PR introduces utility functions that provide developers with a consistent and reliable way of maintaining their contracts. The key goal here is to offer tools that perform the minimal changes necessary to add, replace, or remove facets from a Diamond.
Usage
General
diamondCut
- callsDiamond.diamondCut
printDiamond
- prints a table containing the registered and unregistered selectorsDeployment
addUnregisteredSelectors
- adds unregistered selectors to the Diamond.Upgrade
replaceRegisteredSelectors
- replaces any selector which is registered to a different target addressremoveRegisteredSelectors
- removes the registered selector from the DiamondExamples
Actions