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.

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

refactor: renamed sdkCoin functions to camelCase #50

Closed
wants to merge 3 commits into from

Conversation

MukulKolpe
Copy link

@MukulKolpe MukulKolpe commented Aug 20, 2024

Updated _ERC20ToSdkCoin_ConvertAmount and _SdkCoinToERC20_ConvertAmount to convertAmountFromERC20 and convertAmountToERC20 respectively, aligning them with standard camelCase naming convention to account for standards used in libraries.

closes: #32

Copy link
Member

@srdtrk srdtrk left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution. Looks good, I made an alternate naming suggestion, I don't know what other's think @gjermundgaraba @sangier. We can merge this when the CI passes

src/utils/SdkCoin.sol Outdated Show resolved Hide resolved
src/utils/SdkCoin.sol Outdated Show resolved Hide resolved
Copy link
Contributor

@gjermundgaraba gjermundgaraba left a comment

Choose a reason for hiding this comment

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

@MukulKolpe Not 100% sure why the e2e test fails, but I can check that quickly here.
It looks like the linter is also complaining, however, so try to run just lint and make any changes it suggests :)

@gjermundgaraba
Copy link
Contributor

@MukulKolpe, I just checked it out locally, and I guess the e2e fails are due to an issue in our CI pipeline with some secrets not being able to run. I ran one of the e2e tests myself, so I believe when everything else is fixed here this can be merged anyway :)

@srdtrk
Copy link
Member

srdtrk commented Aug 20, 2024

Yes, created an issue (#54) about this @gjermundgaraba, we can merge this PR without the e2es passing

@MukulKolpe
Copy link
Author

Thanks @gjermundgaraba, @srdtrk!

@gjermundgaraba
Copy link
Contributor

@MukulKolpe, it looks like it is failing on the listing. Try to run just lint locally and implement the suggested changes :)

@crodriguezvega
Copy link
Contributor

Thank you for the PR, @MukulKolpe, but at the end it turned out that we don't need this SDK coin library, so it will be removed.

@MukulKolpe MukulKolpe deleted the refactor_sdkCoin branch August 29, 2024 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Change sdkCoin functions names to camelCase
5 participants