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

Nova setup w/ Saving and user/balance module #6

Merged
merged 6 commits into from
May 10, 2024
Merged

Nova setup w/ Saving and user/balance module #6

merged 6 commits into from
May 10, 2024

Conversation

VaibhavArora19
Copy link
Contributor

Related to #5 issue

@VaibhavArora19 VaibhavArora19 changed the title Nova setup Nova setup w/ Saving and user/balance module May 9, 2024
@VaibhavArora19 VaibhavArora19 marked this pull request as draft May 9, 2024 10:15
thelazyindian
thelazyindian reviewed May 9, 2024
View reviewed changes
Copy link
Contributor

@thelazyindian thelazyindian left a comment

Choose a reason for hiding this comment

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

LGTM! just a small suggestion, let's maintain either snake case or lower camel case in our types, currently its mixed at places. Personally i like lower camel case e.g. fiatPrice instead of fiat_price, also apy instead of APY

@VaibhavArora19 VaibhavArora19 marked this pull request as ready for review May 9, 2024 13:10
@VaibhavArora19
Copy link
Contributor Author

LGTM! just a small suggestion, let's maintain either snake case or lower camel case in our types, currently its mixed at places. Personally i like lower camel case e.g. fiatPrice instead of fiat_price, also apy instead of APY

Fixed!

jakekidd
jakekidd approved these changes May 10, 2024
View reviewed changes
Copy link
Contributor

@jakekidd jakekidd left a comment •
edited
Loading

Choose a reason for hiding this comment

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

lgtm, good work with nest. see 1 comment here about file names

packages/agents/nova/src/common/constants/token-balance.ts
@@ -0,0 +1,22 @@
import { TTokenBalance } from '../types/balance';
Copy link
Contributor

Choose a reason for hiding this comment

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

file names should be camelCase, not use dashes IMO

@sanchaymittal sanchaymittal merged commit 12c7713 into main May 10, 2024
@VaibhavArora19 VaibhavArora19 deleted the nova-setup branch May 11, 2024 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thelazyindian thelazyindian thelazyindian left review comments

@jakekidd jakekidd jakekidd approved these changes

@Yash-Garg Yash-Garg Yash-Garg approved these changes

@sanchaymittal sanchaymittal sanchaymittal approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@VaibhavArora19 @thelazyindian @sanchaymittal @Yash-Garg @jakekidd