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

remove uneeded casting, refractor transfer from mint #461

Merged
merged 4 commits into from
Dec 22, 2022
Merged

remove uneeded casting, refractor transfer from mint #461

merged 4 commits into from
Dec 22, 2022

Conversation

sontrinh16
Copy link
Contributor

@sontrinh16 sontrinh16 commented Dec 5, 2022 •
edited
Loading

Closes: #422

Context and purpose of the change

Fix some audited coding style

Author's Checklist

I have...

  • remove some unnecessary casting to uint64
  • remove some unnecessary else statement
  • remove err check in RedeemStake
  • Change type name for ReinvestAmount
  • Move send stCoins logic to LiquidStake instead of MintAsset

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you!

@github-actions github-actions bot added the Stale label Dec 21, 2022
@sontrinh16
Copy link
Contributor Author

i dont think we can remove the address err check in redeemstake and liquidstake without fixing the test cases right now

@sontrinh16 sontrinh16 added enhancement New feature or request notional Issues that are open to be picked up by notional, if not assigned and removed Stale labels Dec 21, 2022
sampocs
sampocs reviewed Dec 21, 2022
View reviewed changes
Copy link
Collaborator

@sampocs sampocs left a comment

Choose a reason for hiding this comment

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

Thanks for opening this! I agree on moving the code out of the else statement.

Can you elaborate on the motivation behind moving the transfer out of the mint function? If it's because the name is unclear, you could just rename it to MintAndTransferStAsset

@sontrinh16
Copy link
Contributor Author

i think that function should only have 1 responsibility that is minting as it describe, the transfer should be moving out of the funtion. Hmmm but yeah rename it would be clearer too

asalzmann
asalzmann approved these changes Dec 22, 2022
View reviewed changes
Copy link
Contributor

@asalzmann asalzmann left a comment

Choose a reason for hiding this comment

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

lgtm

@sontrinh16 sontrinh16 requested a review from sampocs December 22, 2022 02:49
@sontrinh16
Copy link
Contributor Author

Base on 2 approves, i will merge the PR

@sontrinh16 sontrinh16 merged commit af53f06 into Stride-Labs:main Dec 22, 2022
sontrinh16 added a commit to notional-labs/stride that referenced this pull request Mar 27, 2023
@sontrinh16
remove uneeded casting, refractor transfer from mint (Stride-Labs#461)
7566884 
Co-authored-by: Son Trinh <sontrinh@Sons-MacBook-Pro.local>
@asalzmann asalzmann mentioned this pull request Mar 29, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sampocs sampocs sampocs approved these changes

@asalzmann asalzmann asalzmann approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request notional Issues that are open to be picked up by notional, if not assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AUDIT: Coding style recommendations
3 participants
@sontrinh16 @asalzmann @sampocs