kalepail / stellar-quest-bounties

Stellar Quest Bounties is an extension of the traditional, series based Stellar Quest challenges allowing seasoned and passionate Stellar Questers to continue their journey of education and earning during the "lean times" between Stellar Quest series.
https://quest.stellar.org/bounties
23 stars 27 forks source link

claimable-balances-account-viewer by DFugere1 #158

Closed DFugere1 closed 2 years ago

DFugere1 commented 2 years ago

Link the bounty file

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

Mark your progress

Provide relevant details

PR to the account_viewer https://github.com/stellar/account-viewer-v2/pull/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 commented 2 years ago

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?

DFugere1 commented 2 years ago

Hi,

Here is the PR to the feature.

https://github.com/stellar/account-viewer-v2/pull/375

Let me know if that does not complete the requirement.

Thank you

ElliotFriend commented 2 years ago

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 commented 2 years ago

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

ElliotFriend commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

DFugere1 commented 2 years ago

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

ElliotFriend commented 2 years ago

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!

LorDDark6660 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

DFugere1 commented 2 years ago

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.

DFugere1 commented 2 years ago

The PR has been canceled on the account viewer project.

ElliotFriend commented 2 years ago

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

e47aacd69032607bc6ebe058557a81fdd77a7be179d1d562f15f8ef423d57920