-
Notifications
You must be signed in to change notification settings - Fork 3
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
DecentSablier module #100
DecentSablier module #100
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.
Looks good as a start, I'm cool with merging it as long as these contracts won't be deployed unless finished.
Oh I don't want to merge this yet, it's nowhere near ready |
1617792
to
ff7adfa
Compare
struct Stream { | ||
address sender; | ||
uint40 startTime; | ||
uint40 endTime; | ||
uint40 cliffTime; | ||
bool cancelable; | ||
bool wasCanceled; | ||
address asset; | ||
bool transferable; | ||
uint128 totalAmount; | ||
address recipient; | ||
} |
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.
Let's make sure that this struct (and any others that we've re-created into our various Sablier Interface files) are fully separated from our production contracts. Like, we have our actual contracts, and a handful of Mock contracts to assist with testing. In order to minimize our contract bytecode for the contracts we're deploying to mainnets, let's keep those different structs fully separate and import the necessary files into the necessary contracts.
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.
So actually, since DecentHats_0_1_0
uses this interface, and we want to effectively "lock down" that contract from any changes in the future, we're not able to make any changes to this interface: it'll result in any newly compiled versions of DecentHats_0_1_0
having different bytecode from that which has already been deployed onchain, which will mean our deployment scripts will attempt to re-deploy it and then publish a new contract address in our NPM package. Which will break our front end smartAccount
address prediction code.
This is definitely frustrating but I'm not sure of any way around it. If you want to make edits to this ISablierV2LockupLinerar.sol
file, you need to copy the existing code into a new interface file then make the changes there.
You can see what I mean by attempting to run the deployment scripts on Sepolia (with this PR as-is). You'll see that it deploys a new instance of DecentHats_0_1_0
. We need to avoid that.
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.
Yeahh I see what you mean. Alrighty I'll make the necessary changes
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.
My recommendation is to create a new interface called ISablierV2LockupLinearFull.sol
, and just copy in the whole-ass Interface structure from their official repo, to make sure we've got it properly future proofed.
I wasn't really thinking about this when creating the first ISablierV2LockupLinear.sol
file for DecentHats_0_1_0
, and so now we've gotta do this. Oops!
edit: might need to the same thing with LockupLinear.sol
, because there's a reference to that one in DecentHats_0_1_0
, too.
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.
@adamgall XYZ_Full.sol
with future-proof code sounds like the best plan (and name lol). Will update
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.
@adamgall Yeah this is gonna take a little more thought than a quick copy-paste. Not more difficult, just mismatching variables names and a potential package install (@prb/math/src/UD60x18.sol), or cherry picking which code to copy-paste instead of all.
I'll push what I have meantime, which uses XYZ.sol
and XYZ_2.sol
for locked original, and edited code respectively
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.
@DarksightKellar i've taken the liberty of finishing this cleanup effort. i think it's all good now
See these commits:
…ntracts into decent-sablier-0.1.0
…blierStreamManagement, to also be future-proof
706f190
to
7348526
Compare
d75ed1c
to
7348526
Compare
When proposals are open which will affect Sablier streams, there are certain states that blockchain may get into which will render the created proposal invalid at time of execution, despite being valid at time of creation.
All of those scenarios are due to the specific situation in which a stream which has funds to claim from the stream during the proposal creation, no longer has funds to claim from the stream at proposal execution. This can happen because the role member manually flushed out their stream funds during the time when the proposal is active.
In order to protect against these potential race conditions, we should move the logic to determine whether or not to attempt to withdraw funds from a stream from proposal creation time to proposal execution time.
We think a potential solution to this would be to create and deploy a contract with this logic, then: