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

Add balance and transfer #24

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

leetdev
Copy link

@leetdev leetdev commented Sep 29, 2021

As per https://gitcoin.co/issue/cennznet/grants/10/100026473, added display of balances for CENNZNET accounts as well as simple transfer functionality.

User guide: https://hackmd.io/LKhbIRkjTRiKWRCq5YbDkg?view

Shortcomings:

  • Balances are only loaded from the network when an account view is loaded. Ideally there should be a periodic refresh of balance data.
  • No tests for new functionality.
  • When transferring, there should be a check if the account has enough balance. Currently the transfer can just silently fail in the background in some cases.

To be honest, I was planning to work on these, but just ran out of time. I will gladly fix those issues in the future when I get the chance.

@Amy-Centrality
Copy link

Amy-Centrality commented Oct 4, 2021

Hey @leetdev, thanks for submitting this work! I just checked out your PR branch, and ran into a build error when I ran yarn and then yarn build. Is there anything particular I need to do before I can run the code?

extension-base/src/api/connections.ts:28:49 - error TS2339: Property 'genericAsset' does not exist on type 'DecoratedRpc<"promise", RpcInterface>'.

28         const registeredAssets = (await api.rpc.genericAsset.registeredAssets()).toJSON();
                                                   ~~~~~~~~~~~~

@Amy-Centrality
Copy link

Oh I found out we currently have to use api.rpc like this..
const registeredAssets = (await (api.rpc as any).genericAsset.registeredAssets()).toJSON();

@leetdev
Copy link
Author

leetdev commented Oct 4, 2021

Hi @Amy-Centrality! You're right, I forgot to address that error, which is an issue with the API library. Should I commit a fix in my branch?

@Amy-Centrality
Copy link

Yes please make the change in your branch :)

@Amy-Centrality
Copy link

Hey thanks for fixing it up! I ran into some more errors when I try to load it in Chrome. Have you got a fix for these too?

@polkadot/types-known has multiple versions, ensure that there is only one installed. Either remove and explicitly install matching versions or dedupe using your package manager. The following conflicting packages were found: 4.17.1 <unknown> 4.17.1 <unknown> @polkadot/wasm-crypto has multiple versions, ensure that there is only one installed. Either remove and explicitly install matching versions or dedupe using your package manager. The following conflicting packages were found: 4.0.2 <unknown> 4.0.2 <unknown> @polkadot/keyring has multiple versions, ensure that there is only one installed. Either remove and explicitly install matching versions or dedupe using your package manager. The following conflicting packages were found: 6.11.1 <unknown> 6.11.1 <unknown>

@leetdev
Copy link
Author

leetdev commented Oct 5, 2021

@Amy-Centrality, I've managed to fix some of those errors but unfortunately not all. The main problem is that the cennznet api package has different version requirement of the polkadot api than this extension does. I did upgrade it to the newest alpha version which upgrades some of the dependencies, but there's still a few conflicts that remain. I think this issue should really be addressed on the API level, not here.

@Amy-Centrality
Copy link

I see that makes sense, thank you. I will chat with the team about resolving the issues on the API side :)

@Amy-Centrality
Copy link

Hmm.. do you have a way so I can test this while we wait for the fix on the API side? how did you test it while you developed it?

@leetdev
Copy link
Author

leetdev commented Oct 6, 2021

The extension works just fine for me despite the warnings in the console.

@Amy-Centrality
Copy link

I see, awesome work! Somehow chrome didn't let me load it earlier, but it's all good now. Just one more issue before we can close this bounty :) In the transfer screen, I'm not able to access the Send Funds button because it's been pushed under the window and there's no way to scroll down.. would you be able to fix this please?

image

@leetdev
Copy link
Author

leetdev commented Oct 12, 2021

I've adjusted the layout by moving the asset dropdown right next to the amount input. This fixes the overflow issue.

@Amy-Centrality
Copy link

Uhh.. just ran into one more issue. We will close the bounty and payout the prize to you today :) Really appreciate your work! If you can fix this one issue it'd be amazing, otherwise we'll look into it ourselves :) So when I click on "Create new account", it shows an error like this. Any idea?
image

@leetdev
Copy link
Author

leetdev commented Oct 13, 2021

That was my bad. Should be fixed now.

@leetdev
Copy link
Author

leetdev commented Oct 13, 2021

I'll admit, this is what happens when tests are not being created for new features. I wish I had the time to write tests for my developments, but I just ran out. I would love to keep working on this project and develop the tests, as well as some other things that would improve the added features (such as periodic updates of balances, for example). If you're interested in further contributions from me on this, please let me know.

@jordy25519 jordy25519 mentioned this pull request Oct 18, 2021
@Amy-Centrality
Copy link

Hey @leetdev, I asked our developers to review this PR, and they came back with some feedback. I know that we've already closed this bounty, but I hope you could help with these since you are the most familiar with this part of the code. (We had to close the bounty due to time limits from GitCoin)

We should ideally fix the following before merging:

  1. Balances are only loaded from the network when an account view is loaded. But there should be a periodic refresh of balance data.
  2. No tests for new functionality.
  3. When transferring, there should be a check and error message, if the account has enough balance. Currently, the transfer can just silently fail in the background in some cases.

Issue number 1 is very important, as it currently won't show the balance update. You can subscribe to change in balance for cennz and cpay following this example,
https://github.com/cennznet/api.js/blob/8f0c00f97a8036299a8035d8d558ac6833dce733/docs/examples/promise/03_listen_to_balance_change/index.js

@leetdev
Copy link
Author

leetdev commented Oct 21, 2021

Hi @Amy-Centrality!

I can work on those issues next week as I'm kind of preoccupied at the moment.

I can definitely fix issue numbers 1 and 3 quite expediently, but issue 2 (writing tests) is a bit more time consuming. I can't promise right now when I might have the time to get around to it... But I'll gladly work on the other issues next week.

@Amy-Centrality
Copy link

Awesome! Thanks very much @leetdev! :)

@leetdev
Copy link
Author

leetdev commented Jul 6, 2022

Hi @Amy-Centrality,

For some reason I was sure I finished working on this, but just found it in my open PRs list, I must have completely blanked! My apologies, I have no idea how it could have happened...

I have some free time right now, I will see if I can finally clear this up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants