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

fix: user get balance #50

Closed
wants to merge 8 commits into from
Closed

fix: user get balance #50

wants to merge 8 commits into from

Conversation

0x3bfc
Copy link
Contributor

@0x3bfc 0x3bfc commented Jun 29, 2021

closes #49

@0x3bfc 0x3bfc requested a review from paouvrard as a code owner June 29, 2021 13:13
@0x3bfc 0x3bfc requested a review from mfornet June 29, 2021 13:13
mfornet
mfornet approved these changes Jun 29, 2021
View reviewed changes
Copy link
Contributor

@mfornet mfornet left a comment

Choose a reason for hiding this comment

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

LGTM, can you address failing CI

@0x3bfc 0x3bfc changed the title Fix/user get balance fix: user get balance Jun 30, 2021
@0x3bfc
Copy link
Contributor Author

0x3bfc commented Jun 30, 2021

can you address failing CI

Yeah sure

@paouvrard
Copy link
Member

Should we move getBalance to getBalance.js then since it is not metadata ? and rename getErc20Data to getMetadata?
For some context, I think getErc20Data grouped everything together as a convenience to the rainbow-bridge-frontend, https://github.com/aurora-is-near/rainbow-bridge-frontend/blob/06ca162327e16288b389b8ee28843a48d22edecd/src/js/utils.js#L18 but since getAllowance was also added separately, it makes send to refactor it.

@0x3bfc
fix: user get balance
25fb5cb 
style: update documentation

style: follow commitlint convension

fix: releasing process error
paouvrard
paouvrard approved these changes Jul 5, 2021
View reviewed changes
Copy link
Member

@paouvrard paouvrard left a comment

Choose a reason for hiding this comment

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

LGTM thank you for refactoring! Please squash your commits to remove the CI fixing. About the commit format, fix: Commit msg... is for code patches which require a patch release and would appear as a bug fix in the changelog. For commits which don't require a new release version, build, chore, ci, docs, refactor... can be used. https://www.conventionalcommits.org/en/v1.0.0/

packages/nep141-erc20/tsconfig.json Show resolved Hide resolved
@paouvrard
Copy link
Member

I think we can close this PR because the library is now using ethers and I will be making this refactor in this PR: #58

@0x3bfc
Copy link
Contributor Author

0x3bfc commented Jul 21, 2021

I think we can close this PR because the library is now using ethers and I will be making this refactor in this PR: #58

Please close it if you feel that it doesn't add to main branch or the refactor you have in the PR #58 :)

@paouvrard paouvrard closed this Aug 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mfornet mfornet mfornet approved these changes

@paouvrard paouvrard paouvrard 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.

getBalance is not a generic ERC20 metadata
3 participants
@0x3bfc @paouvrard @mfornet