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

Adds shielded button and functionality (transparent -> orchard[unified]) to zingo-pc #67

Conversation

elijahhampton
Copy link

@elijahhampton elijahhampton commented Jun 4, 2023

Proposed changes
Creates new rpc functions doShield and shieldBalanceToOrchard. Adds shieldBalanceToOrchard to . Adds shieldBalanceToOrchard to .

General Notes
Note: This merge request may only be seen as a draft as there are some questions that will probably need to be addressed:

  1. Is this the desired placement of the Shield button?
  2. The UI freezes during the "shielding" operation. I can tackle a general UI loader in this PR or leave the PR to this one issue.
  3. I used my prettier config on these files without much thought. I can revert the prettier changes or create a new PR without the prettier changes.

Visuals
New Button (This button is "Shield Balance" now)

Captura de pantalla 2023-06-03 a la(s) 6 26 14 p m

Working Error State

Captura de pantalla 2023-06-03 a la(s) 6 26 33 p m

See list of transactions here. This involves a number of transactions I made sending ZEC to my transparent address and shielding it.

https://blockchair.com/zcash/address/t1Zs9uxRyN6Knd6kDHfRKjLjn5JchnAPFN8

Transparent Balance After Shielding

Captura de pantalla 2023-06-04 a la(s) 2 20 22 p m

Orchard (Unified) Balance After Shielding

Captura de pantalla 2023-06-04 a la(s) 2 20 28 p m

Elijah Hampton added 2 commits June 4, 2023 13:42
…hieldBalanceToOrchard to <AddressBook />. Adds shieldBalanceToOrchard to <Receive />.
@elijahhampton elijahhampton changed the title Creates new rpc functions doShield and shieldBalanceToOrchard. Adds s… Adds shielded button and functionality (transparent -> orchard[unified]) to zingo-pc Jun 4, 2023
@juanky201271
Copy link
Contributor

Aja!!!!!.... Thanks for your contribution... I need to find time to review this PR, I'll do tomorrow hopefully. Looks good though.

@juanky201271 juanky201271 self-requested a review June 4, 2023 18:21
@juanky201271 juanky201271 linked an issue Jun 4, 2023 that may be closed by this pull request
@elijahhampton
Copy link
Author

elijahhampton commented Jul 1, 2023

Updates completed. Change "shield balance" to "shield balance to orchard".

Adds functionality to shield sapling to orchard separately from transparent.

@juanky201271

@juanky201271
Copy link
Contributor

Updates completed. Change "shield balance" to "shield balance to orchard".

Adds functionality to shield sapling to orchard separately from transparent.

@juanky201271

Looks pretty good, I need to test it in local before merge it...

@@ -171,6 +175,16 @@ const AddressBlock = ({
View on explorer <i className={["fas", "fa-external-link-square-alt"].join(" ")} />
</button>
)}
{type === AddressType.transparent && (
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need to check if there is something to shield, if the transparent balance is 0 make no sense to show this button.

Shield Balance To Orchard
</button>
)}
{type === AddressType.sapling && (
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need to check if there is something to shield, if the sapling balance is 0 make no sense to show this button.

const result = await this.rpc.shieldSaplingBalanceToOrchard()

if (!result.includes('txid')) {
throw new Error(result)
Copy link
Contributor

@juanky201271 juanky201271 Jul 2, 2023

Choose a reason for hiding this comment

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

I think the user needs some feedback here, you can use openerrormodal here if the result have no txid
And the user needs some feedback even if the transaction finished successfully... using this txid as well.

would be good if you use the pattern that in the send process... since these two process are similar, creating both a new transaction.

const result = await this.rpc.shieldTransparentBalanceToOrchard()

if (!result.includes('txid')) {
throw new Error(result)
Copy link
Contributor

Choose a reason for hiding this comment

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

same here.

@juanky201271
Copy link
Contributor

I resolved the conflicts...

@juanky201271
Copy link
Contributor

@elijahhampton How's it going? If you are really busy I can take this PR from here & finish it in your branch... I'm happy to help.
And Thanks.

cc: @Edicksonjga

@juanky201271
Copy link
Contributor

@elijahhampton Well, I'm going to wait a little bit. I tried to contact with you by several channels... and no luck. Please tell me what do you think... about this PR progression...

@juanky201271 juanky201271 merged commit 31c0014 into zingolabs:dev Jul 25, 2023
@elijahhampton
Copy link
Author

@elijahhampton Well, I'm going to wait a little bit. I tried to contact with you by several channels... and no luck. Please tell me what do you think... about this PR progression...

Thank you for finishing the PR. I responded to your message through Element.

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.

Bugs - Payment URIs not working, Transaction Details and Shield funds New button to shield all funds.
3 participants