Conversation
|
No dependency changes detected. Learn more about Socket for GitHub ↗︎ 👍 No dependency changes detected in pull request |
andrew-fleming
left a comment
andrew-fleming
left a comment
There was a problem hiding this comment.
Very nice PR, @immrsd! I left a few questions and comments
| import { durationToTimestamp } from './utils/duration'; | ||
| import { toUint, validateUint } from './utils/convert-strings'; | ||
|
|
||
| export type VestingSchedule = 'linear' | 'custom'; |
There was a problem hiding this comment.
Since we already have the "steps" schedule implemented as a mock and as an example in "Usage", do you think it's worth adding the option here?
There was a problem hiding this comment.
Actually, I don't reckon it's worth it. The steps schedule is mostly for demonstration purposes, it's kinda naive and I expect it to be used almost never or very rarely
| <HelpTooltip link="https://docs.openzeppelin.com/contracts-cairo/access#ownership_and_ownable"> | ||
| Simple mechanism with a single account authorized for all privileged actions. | ||
| </HelpTooltip> | ||
| </label> | ||
| <label class:checked={opts.schedule === "custom"}> | ||
| <input type="radio" bind:group={opts.schedule} value="custom"> | ||
| Custom | ||
| <HelpTooltip link="https://docs.openzeppelin.com/contracts-cairo/access#role_based_accesscontrol"> | ||
| Flexible mechanism with a separate role for each privileged action. A role can have many authorized accounts. | ||
| </HelpTooltip> |
There was a problem hiding this comment.
The tooltips need to be fixed
There was a problem hiding this comment.
If we don't want to include the steps vesting option, I think we can at least direct users to it as an example when hovering the Custom tooltip
| const match = duration.trim().match(durationPattern); | ||
|
|
||
| if (!match) { | ||
| if (!match || match.length < 2) { |
There was a problem hiding this comment.
Are there cases where a happy match also has match.length < 2?
There was a problem hiding this comment.
No, since later on we access the 1st and the 2nd items of the match array in unsafe manner. Either way it'll result in a runtime exception, but it's better to have a straightforward error than index-out-of-bounds one
ericglau
left a comment
ericglau
left a comment
There was a problem hiding this comment.
Looks good, thanks! Just a few suggestions.
ericnordelo
left a comment
ericnordelo
left a comment
There was a problem hiding this comment.
The PR and generated code look good, but I wonder if there's anything we can do to make the date input UI compatible with the rest of the inputs, since it looks kind of broken cc @ericglau
|
Applied styling to the date input. cc @ericnordelo
|
|
Results of test project compilation with only Vesting being enabled: 
|
…nto vesting-wallet
4 participants
No description provided.