sablier-labs / flow

🍃 Smart contracts of the Sablier Flow protocol.
Other
10 stars 2 forks source link

feat: add table creator script contract #287

Closed andreivladbrg closed 1 month ago

andreivladbrg commented 1 month ago

Similar to: https://github.com/sablier-labs/v2-core/pull/1049

I couldn't find a better name for the contract, so I named it TableCreator (if you better suggestions lmk). I also didn’t think it was appropriate to include all the logic in the Base_Script contract.

Other changes

build: add more chains in foundry toml build: add etherscan config

andreivladbrg commented 1 month ago

What do we do when chain details are not found in these tables? Do we re-deploy the contracts by adding these chain details such as explorer and chain name or let it go like that? IMO we should revert the deployment.

i am not sure i understand completely what you mean? if the chain is not found, it will use the DEFAUL DEPLOYER as the admin and then on the table, it would simply print the chain Id (number) and with the missing explorer. why would you want to revert? it would still log the relevant informations in the terminal

2. Have you tested these scripts?

yes, i have, running the CLI forge script script/DeployFlow.s.sol --rpc-url sepolia it creates the file not-deterministic.md:

## Sepolia

| Contract          | Address                                                                                                                       | Deployment                                                          |
| :---------------- | :---------------------------------------------------------------------------------------------------------------------------- | :------------------------------------------------------------------ |
| SablierFlow       | [0x83dd52fca44e069020b58155b761a590f12b59d3](https://sepolia.etherscan.io/address/0x83dd52fca44e069020b58155b761a590f12b59d3) | [v1.0.0](https://github.com/sablier-labs/v2-deployments/tree/main/) |
| FlowNFTDescriptor | [0x8224eb5d7d76b2d7df43b868d875e79b11500ea8](https://sepolia.etherscan.io/address/0x8224eb5d7d76b2d7df43b868d875e79b11500ea8) | [v1.0.0](https://github.com/sablier-labs/v2-deployments/tree/main/) |

and if you simply run it multiple times, the previous deployment won't be delated


i have pushed a new commit to address your feedback, lmk if it looks good

smol-ninja commented 1 month ago

There should be deployments/ folder as as a part of this PR. Otherwise the forge script script/DeployFlow.s.sol throws execution reverted: vm.writeLine: No such file or directory (os error 2) (gas: 3111719)

I suppose an empty folder won't be allowed by the git. In that case, we should document is in the contributing guideline or readme. Any other ideas?

smol-ninja commented 1 month ago

There should be deployments/ folder as as a part of this PR. Otherwise the forge script script/DeployFlow.s.sol throws execution reverted: vm.writeLine: No such file or directory (os error 2) (gas: 3111719) I suppose an empty folder won't be allowed by the git. In that case, we should document is in the contributing guideline or readme. Any other ideas?

Regarding this, how about putting these markdown files in the script folder itself and add them into gitignore.

andreivladbrg commented 1 month ago

There should be deployments/ folder as as a part of this PR. Otherwise the forge script script/DeployFlow.s.sol throws execution reverted: vm.writeLine: No such file or directory (os error 2) (gas: 3111719)

my bad, should have also mentioned in this PR as well, please see the OP in the lockup repo: https://github.com/sablier-labs/v2-core/pull/1049#issue-2564368220

also the natspec in the contract should say this

Regarding this, how about putting these markdown files in the script folder itself and add them into gitignore

hmm, what if we use appendToFileDeployedAddresses only on the run() script functions, which is meant to be executed by us (sablier team) which should know that deploying a contract means we should update the documentation as well with the new contracts and the it requires a specific table format. wdyt?

smol-ninja commented 1 month ago

what if we use appendToFileDeployedAddresses only on the run() script functions

Works both ways to me (you can decide). But even if its meant to be executed by us, given that its a public repo, any information required to run any command (such as forge script) should be a part of the contributing guidelines. Else a user jump into our discord and ask that why this particular command not working and we end up answering that he needs to create deployments folder first.

which is meant to be executed by us (sablier team) which should know that deploying a contract means we should update the documentation as well with the new contracts and the it requires a specific table format

Secondly, even if we expect ourselves to be aware of these things, we should also document it for future reference or external developer.

andreivladbrg commented 1 month ago

fair enough, let's move them into scripts dir

a18a624ecbdfd576ebedfa5968e245fc2a538b3a

andreivladbrg commented 1 month ago

i have addressed eveything now, thanks for the feedback

does it look good to be merged? @smol-ninja