Conversation
wischli
left a comment
wischli
left a comment
There was a problem hiding this comment.
LGTM except for the potential incorrect VERSION. Script also worked for me locally, great job!
script/PoolHooks.s.sol
Outdated
| contract PoolHooks is JsonRegistry, GraphQLQuery, CreateXScript { | ||
| using stdJson for string; | ||
|
|
||
| bytes32 constant VERSION = "1"; |
There was a problem hiding this comment.
I think this should be "v3.1" as configured in env/*.json
| } else if (currentHook == fullRestrictionsHook) { | ||
| _deployFullRestrictions(token.poolId, token.scId, poolEscrowAddr); | ||
| } else { | ||
| console.log("Hook is not freelyTransferable or fullRestrictions, skipping"); |
There was a problem hiding this comment.
I suppose none of the pools use FreezeOnly or RedemptionRestrictions hooks?
script/PoolHooks.s.sol
Outdated
| string memory saltName = string.concat("fullRestrictions-", vm.toString(PoolId.unwrap(poolId))); | ||
| bytes32 salt = makeSalt(saltName, VERSION, deployer); | ||
|
|
||
| hook = FullRestrictions( |
There was a problem hiding this comment.
Wondering if we should make the script idempotent by adding a check whether it has been deployed already.
There was a problem hiding this comment.
Yes maybe a good idea. We may need to run the script again before we have the factory in place.
There was a problem hiding this comment.
That would be very useful yes!
lemunozm
left a comment
lemunozm
left a comment
There was a problem hiding this comment.
Except my two comments, the whole process looks really nice!
|
|
||
| sleep 3.0 # Wait ensuring Anvil is up | ||
|
|
||
| SENDER="0xc1A929CBc122Ddb8794287D05Bf890E41f23c8cb" |
There was a problem hiding this comment.
As a note. This sender is the admin from testnet.
There was a problem hiding this comment.
ah yes I just picked a random address for testing
| bytes32 salt = makeSalt(saltName, VERSION, deployer); | ||
|
|
||
| hook = FreelyTransferable( | ||
| create3( |
There was a problem hiding this comment.
I really think we don't need create3 if we deploy with a custom deployer. We're not gaining anything, and can be confused when in other chains they have different addresses for the same pools.
I would go only with create3 if we can ensure that we always have the same addresses in all chains.
The only way to have all hooks with the same addresses for the same pools is using a factory IIUC.
There was a problem hiding this comment.
Right now, if you use the same account to deploy on all networks, the hooks for the same pool will all have the same address. If we need to deploy more for the same pool again in the future, we could use the same account again to get the same addresses AFAIK.
There was a problem hiding this comment.
The problem is that future hooks will be deployed by pool managers, not us. This is only a first time initialization by us
There was a problem hiding this comment.
True, though I'm not sure it's that important that between now and then, the addresses are the same. Just that now they are the same for the same pool seems convenient.
Maybe @hieronx has thoughts on this?
4 participants
Product requirements
Mostly for review. Don't think we want to merge this
Design notes
TODOs