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

claimable-balances-account-viewer by DFugere1 #158

Closed
3 tasks done
DFugere1 opened this issue Jan 21, 2022 · 15 comments
Closed
3 tasks done

claimable-balances-account-viewer by DFugere1 #158

DFugere1 opened this issue Jan 21, 2022 · 15 comments

Comments

@DFugere1
Copy link
Contributor

DFugere1 commented Jan 21, 2022

Link the bounty file

https://github.com/tyvdh/stellar-quest-bounties/blob/main/bounties/level-1/claimable_balances_account_viewer.md

Mark your progress

  • 🔵  Started working
  • 🟢  Ready for review
  • 🟣  Review completed

Provide relevant details

PR to the account_viewer
stellar/account-viewer-v2#375

Demo:
https://elegant-perlman-79e8d5.netlify.app/?testnet=true

Greeting,

I made a POC for claiming balance on the account viewer. The PR should be available tomorrow.
Before submitting, I would like to discuss the term of completeness.

If a Claimable Balance (CB) is a non native asset, shall we go ahead and offer to add the asset trust line to the wallet. Or is this feature in a following ticket.

Thank you.

@ElliotFriend
Copy link
Collaborator

Hi, there! Thanks for jumping on this one!

I'd suggest when a user clicks "claim balance" (or whatever you call it) in your viewer, the app should automatically:

  1. Check if it's a non-native asset (this has probably already done by this point in the process, but just in case)
  2. If so, check if the user has a trustline to that asset already
  3. If not, just go ahead and add the appropriate trustline operation(s) to the claim transaction you're building
  4. Request the signature and/or submit to the network

I don't see a need for it to be an "offer" they could opt in or out of. If there's no trustline the claim will fail. If they want the asset, I think we can safely assume they want the trustline, too. I think this is how most of the wallets work?

Does that all make sense?

@github-actions github-actions bot added review and removed work labels Jan 23, 2022
@DFugere1
Copy link
Contributor Author

Hi,

Here is the PR to the feature.

stellar/account-viewer-v2#375

Let me know if that does not complete the requirement.

Thank you

@ElliotFriend
Copy link
Collaborator

Do you have a preview setup where we can see/test your change? A standalone version of the account viewer that includes your changes would be required for the bounty to be considered complete, as per the second bullet point here.

@DFugere1
Copy link
Contributor Author

You are right, I forgot about that. I edited my post and added a demo.

@ElliotFriend
Copy link
Collaborator

Thanks for adding the preview. Can confirm that it worked to claim a random balance on the testnet. Good job! I tried using a testnet account that's setup for the YieldBlox public testnet beta, but it threw some kind of error after signing the transaction. I'm willing to bet it was something weird with that account, though, rather than your preview. Worked like a charm on a vanilla testnet account.

Let's see if we can get some code reviewers over here to give it a look-through.

@BlackBadPinguin
Copy link
Contributor

BlackBadPinguin commented Jan 25, 2022

I also tried to claim a claimable balance with the preview on the Testnet, however I also got an error. I tried signing the transaction with Albedo and I think it fails because Albedo tries to sign it for the PubNet even tho the Transaction is on the Testnet. You can also see this in this picture:
image
When you change the Network identifier to TestNet, the Preview should work with Albedo.

@DFugere1
Copy link
Contributor Author

I will take a look at the problem with the network when using Albedo, thank you for bringing it up!

@DFugere1
Copy link
Contributor Author

DFugere1 commented Jan 26, 2022

After some testing, the issue is also appening on the master branch of the application. I will try to investiguate it further?

@ElliotFriend
Copy link
Collaborator

ElliotFriend commented Jan 27, 2022

I see you are correct! Very interesting. I'll add an issue to the repository, and we won't count it against your implementation. Thanks for helping to catch this!

Here's the issue: stellar/account-viewer-v2#376 @DFugere1, you might want to check that and add any information I missed or got wrong.

Thanks!

@ElliotFriend ElliotFriend changed the title Should we add trustline if needed claimable-balances-account-viewer by DFugere1 Jan 28, 2022
@LorDDark6660
Copy link
Contributor

LorDDark6660 commented Jan 28, 2022

With Freighter everithing works fine, i think albedo have some issue on testnet, i am testing YieldBlox with albedo and Freighter and albedo have the same issue like here. I would also not add an accept trustline button, in case of a large number of CBs it is a waste of time. Maybe it would not be bad to create a separate button for CB, something like on the picture, but nicer 😉. grey inactive(0 CB), violet active (pending CB)
IMG_20220128_013720

@BlackBadPinguin
Copy link
Contributor

I now also tested it with Freighter and can confirm it works as intended, so the issue with Albedo is indeed in the Account Viewer itself, as far as I tested it everything works fine, as @LorDDark6660 already mentioned, a separate CB Button would be nice, however, it isn't mentioned in the quest afaik. So I would say the Quest is done and ready for a reward.

@BlackBadPinguin
Copy link
Contributor

Also, don't forget to add a PR for adding your Address to the ADDRESSES.yml file.

@DFugere1
Copy link
Contributor Author

Thank you, I submitted the PR. I will still work on the PR made to the account-viewer project and try to get it up to standard.

@github-actions github-actions bot added award and removed review labels Jan 29, 2022
@DFugere1
Copy link
Contributor Author

DFugere1 commented Feb 7, 2022

The PR has been canceled on the account viewer project.

@ElliotFriend
Copy link
Collaborator

Thanks for the update on that. Still a good experience, and certainly worthy of a bounty reward! Great job and excellent contribution!

e47aacd69032607bc6ebe058557a81fdd77a7be179d1d562f15f8ef423d57920

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

No branches or pull requests

4 participants