-
Notifications
You must be signed in to change notification settings - Fork 7
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
Some additional validation and UI fixes for Sablier Proposal builder #2352
Conversation
✅ Deploy Preview for decent-interface-dev ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@mudrila can you please describe how these changes address & fix the issues that Shutter is experiencing? How did you test these changes? |
@@ -565,7 +570,9 @@ function StreamBuilder({ | |||
isRequired | |||
value={tranche.duration.bigintValue} | |||
placeholder={(SECONDS_IN_DAY * 365).toString()} | |||
decimalPlaces={1} | |||
decimalPlaces={0} |
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.
@decentdao/engineering This was causing problems with durations being multiplied by 10
Removing this results in creating stream with proper durations
if (!stream.recipientAddress || !stream.tokenAddress || !stream.totalAmount) { | ||
return true; | ||
} | ||
const invalidTranches = stream.tranches.filter((tranche, index) => { |
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.
@decentdao/engineering Potentially - there's so much stuff to cover, but I'm not sure whether it's worth that given this feature is custom implementation specifically for Shutter. Potential things to validate against:
- Checking for treasury balance and disabling the button(or showing warning?) if treasury balance is less than sum of tokens in multiple streams
- Checking tranches amounts
- Showing total duration of the stream
On the other hand - well, once I thought that it's not reasonable - and here we are, with this PR 🤣
Any thoughts?
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 validation looks like the appropriate level of validation to me.
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.
(nitpick) could these filters be .some
?
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.
Approving this, assuming the code has been manually tested (by you @mudrila), using the inputs that Shutter shared with you that they'll be using on Mainnet.
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.
Nicpick but otherwise code looks good to me.
if (!stream.recipientAddress || !stream.tokenAddress || !stream.totalAmount) { | ||
return true; | ||
} | ||
const invalidTranches = stream.tranches.filter((tranche, index) => { |
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.
(nitpick) could these filters be .some
?
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.
LGTM
Testing these changes: