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

Feature/ssv deposit balance #160

Merged
merged 10 commits into from
Oct 21, 2022
Merged

Feature/ssv deposit balance #160

merged 10 commits into from
Oct 21, 2022

Conversation

ccali11
Copy link
Contributor

@ccali11 ccali11 commented Oct 17, 2022

@shanejearley - Please see PR for #24. Worth reviewing a bit together to discuss a few findings as well as unresolved notes that I have. Here is a checklist we can review:

  • Resolve terminal error: Error: Transaction reverted: function selector was not recognized and there's no fallback function
    • Submit a bug report and tag Shane
  • Potentially better handle promise returned from await ssv.connect(provider).getUserBalanceForPool(userAddress, pool) (including current for loop implementation in front-end component)
  • Review with Chris where would be best to declare the refs we want exposed on front-end (inside the composable function or outside)
  • Discuss time delay required to submit multiple transactions
  • I noticed a new directory that got added to my branch at some point (perhaps during a Live Share). Perhaps you know where it came from and can tell me if I should include it in this PR: services/auth/
  • Typing errors
  • Add Ledger and WalletConnect

Looking forward to catching up!

apps/web/src/types/Pool.ts Outdated Show resolved Hide resolved
apps/web/src/composables/wallet.ts Outdated Show resolved Hide resolved
apps/web/src/composables/wallet.ts Outdated Show resolved Hide resolved
apps/web/src/composables/wallet.ts Show resolved Hide resolved
@shanejearley
Copy link
Contributor

@ccali11 the last two commits add ethers.Signer implementations for Ledger and WalletConnect in the common directory. Can you pull the latest down and test the WalletConnect message signer method in your UI (I don't have WalletConnect set up)?

Let's meet on this since I probably need to explain myself (went a bit overboard, but this should standardize things well moving forward).

@shanejearley
Copy link
Contributor

Approved and suitable to merge once we confirm all ethers wallets are working as expected for check balance, deposit, and sign message.

@ccali11 ccali11 merged commit 3cd0add into develop Oct 21, 2022
@ccali11 ccali11 deleted the feature/ssv-deposit-balance branch October 21, 2022 18:54
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