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.
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
Add timelock component #996
Add timelock component #996
Changes from 70 commits
c39a881
2caf277
54db1f7
c48521c
61a9258
df1d886
40ad554
f009bd9
43bf794
83525ba
587a29f
e50b1db
4b3a44c
169397c
07bb530
65d2be6
6f3a47f
0e3b35b
2c1186e
fcd0331
9450530
290ecbc
c5d4102
ba28288
4540db2
cc70677
aab9245
b19c193
b470278
269ea6e
08908b5
090d1a3
2b11d3e
7f5a848
2b42f6d
ae31e78
ae1b474
d12be8e
98fb423
c8efa81
7054fcd
41681d9
7144eec
0070607
4a4d495
9ce8d02
6c67531
f5b484a
7527d0d
cd0352c
f638c82
a51aae3
a3c03d8
39561ee
8764889
6cacdc7
e4fd560
3583d38
e878cae
70f44f4
4018bbe
29e0865
ca5b0dd
0909f9f
3edf90f
ae45e5b
3e533e1
e6dc707
b92165a
3bc0a6b
8b29788
7dddac9
996123b
a48b2a3
309b4d0
0f89aa0
2278f4f
d366e81
9900a8c
9686d22
e3e62ea
ac9c4b8
9350e50
9a7f3bf
a11a6e7
1487b9f
025bc6d
7f09ecc
ab0025a
de47270
e97407e
f5c1e63
3d96bf2
6352975
ad47074
858bb94
6c37eb8
a4c1f29
09a11b0
986e888
03f0a22
d4d10df
862a5c4
2de0eee
edfe5fb
0f24416
c485f78
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 can remove these from the timelock component, since they are separated components, and can be optionally added in the preset if required. Wdyt?
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 will definitely reduce some of the dependency noise in the implementation if we remove them. My concern, though, is that I'm not sure it's best to make this opt in as I'd expect token receiver support out of the box (especially with erc1155 transfers). Do you disagree and think opt in is 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.
To me, opt-in is better. I don't see why every Timelock should be a valid token receiver, since usually Timelocks own contracts for delayed execution of admin actions, and this use case doesn't require being able to receive tokens. It is true that a Timelock could be helpful to "scrow" tokens, but that's why opt-in makes sense IMO. Curious about @Amxx thoughts on this.
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 would tend toward not including it by default but not because I don't think its usefull. In fact I think most (if not all) timelock should have it.
But the thing is, if you put the 721 hook, then you also need to put the 1155 hook (because why not?) ... but then are there any other hooks you should have ? Possibly ...
If you put them, you are removing that responsability from the user, and when something ends up missing that is on you. If you don't put any, and give that responsability to the user (or to the preset), then you separate the concerns better. On one hand you have the timelock logic that should probably not change a lot, on the other you have the hooks, were new stuff might be included more often.
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 points, I'm convinced. Thanks for the feedback! Will update accordingly