Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Vesting Component #1116

Merged
merged 32 commits into from
Sep 18, 2024
Merged

Vesting Component #1116

merged 32 commits into from
Sep 18, 2024

Conversation

immrsd
Copy link
Collaborator

@immrsd immrsd commented Aug 26, 2024

Solves #334

PR Checklist

  • Tests
  • Documentation (to be added in a separate PR)
  • Added entry to CHANGELOG.md
  • Tried the feature on a public network

@immrsd immrsd self-assigned this Aug 26, 2024
ericnordelo
ericnordelo reviewed Aug 27, 2024
View reviewed changes
Copy link
Member

@ericnordelo ericnordelo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good @immrsd. Left some comments already. I will iterate again over the component and the tests after the current comments are addresses, just to avoid having to many noise.

packages/finance/Scarb.toml Outdated Show resolved Hide resolved
packages/finance/src/tests/mocks.cairo Outdated Show resolved Hide resolved
packages/finance/src/tests/mocks/vesting_mocks.cairo Outdated Show resolved Hide resolved
packages/finance/src/vesting.cairo Show resolved Hide resolved
packages/finance/src/tests/mocks/vesting_mocks.cairo Outdated Show resolved Hide resolved
packages/token/src/erc20/utils.cairo Outdated Show resolved Hide resolved
packages/finance/src/vesting/vesting.cairo Outdated Show resolved Hide resolved
packages/finance/src/vesting/vesting.cairo Outdated Show resolved Hide resolved
packages/finance/src/vesting/vesting.cairo Show resolved Hide resolved
packages/finance/src/vesting/vesting.cairo Show resolved Hide resolved
andrew-fleming
andrew-fleming reviewed Aug 29, 2024
View reviewed changes
Copy link
Collaborator

@andrew-fleming andrew-fleming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR looks really good! I left some comments, questions, and suggestions :)

packages/finance/src/lib.cairo Outdated Show resolved Hide resolved
packages/finance/src/tests.cairo Show resolved Hide resolved
packages/finance/src/tests/test_vesting_steps.cairo Outdated Show resolved Hide resolved
packages/token/src/erc20/utils.cairo Outdated Show resolved Hide resolved
packages/finance/src/tests/test_vesting_linear.cairo Outdated Show resolved Hide resolved
packages/finance/src/tests/test_vesting_linear.cairo Outdated Show resolved Hide resolved
packages/finance/src/tests/test_vesting_linear.cairo Outdated Show resolved Hide resolved
packages/presets/src/tests/test_vesting.cairo Outdated Show resolved Hide resolved
packages/presets/src/tests/test_vesting.cairo Outdated Show resolved Hide resolved
packages/finance/src/tests/test_vesting_linear.cairo Outdated Show resolved Hide resolved
ericnordelo
ericnordelo reviewed Aug 29, 2024
View reviewed changes
Copy link
Member

@ericnordelo ericnordelo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @immrsd, I see a few marked-as-resolved conversations but I can't see the updates in the PR. Let's mark as resolved when the fix has been pushed, because if not is easy at least for me to miss what the fix was.

@ericnordelo
Copy link
Member

Hey @immrsd, I see a few marked-as-resolved conversations but I can't see the updates in the PR. Let's mark as resolved when the fix has been pushed, because if not is easy at least for me to miss what the fix was.

Never mind, I can see the fix now, I'm not sure why it was not showing before.

andrew-fleming
andrew-fleming reviewed Sep 9, 2024
View reviewed changes
Copy link
Collaborator

@andrew-fleming andrew-fleming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good! I left a question

packages/finance/src/vesting/vesting.cairo Show resolved Hide resolved
ericnordelo
ericnordelo approved these changes Sep 11, 2024
View reviewed changes
Copy link
Member

@ericnordelo ericnordelo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great @immrsd! Left a couple of small suggestions, but besides I think is good to go.

packages/presets/src/vesting.cairo Outdated Show resolved Hide resolved
packages/finance/src/vesting/vesting.cairo Outdated Show resolved Hide resolved
packages/finance/src/vesting/vesting.cairo Show resolved Hide resolved
@ericnordelo
Copy link
Member

We also need to add a README to the new finance package, and update the Scarb.toml to be consistent with the workspace improvements PR recently merged.

ericnordelo
ericnordelo reviewed Sep 17, 2024
View reviewed changes
packages/finance/README.md Outdated Show resolved Hide resolved
packages/finance/README.md Outdated Show resolved Hide resolved
packages/finance/Scarb.toml Show resolved Hide resolved
andrew-fleming
andrew-fleming approved these changes Sep 18, 2024
View reviewed changes
Copy link
Collaborator

@andrew-fleming andrew-fleming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few minor suggestions. Otherwise, LGTM! Nice work :)

packages/finance/src/tests/mocks/erc20_mocks.cairo Show resolved Hide resolved
packages/finance/src/vesting/vesting.cairo Outdated Show resolved Hide resolved
packages/finance/src/vesting/vesting.cairo Outdated Show resolved Hide resolved
packages/finance/src/vesting/vesting.cairo Outdated Show resolved Hide resolved
@immrsd immrsd merged commit 5cea01f into OpenZeppelin:main Sep 18, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericnordelo ericnordelo ericnordelo approved these changes

@andrew-fleming andrew-fleming andrew-fleming approved these changes

Assignees

@immrsd immrsd

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@immrsd @ericnordelo @andrew-fleming