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.

Sign up for GitHub

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

Jump to bottom

fix: enables unlock and withdrawal from Lockdrop accounts #1279

Merged
merged 6 commits into from
May 8, 2024
Merged

fix: enables unlock and withdrawal from Lockdrop accounts #1279

merged 6 commits into from
May 8, 2024

Conversation

impelcrypto
Copy link
Member

@impelcrypto impelcrypto commented May 5, 2024 •
edited

Pull Request Summary

  • fix: enables unlock and withdrawal from Lockdrop accounts

Check list

  • contains breaking changes
  • adds new feature
  • modifies existing feature (bug fix or improvements)
  • relies on other tasks
  • documentation changes
  • tested on mobile devices

This pull request makes the following changes:

@github-actions GitHub Actions
Copy link

github-actions bot commented May 5, 2024 •
edited

Visit the preview URL for this PR (updated for commit 5ef13a0):

https://astar-apps--pr1279-fix-lockdrop-unlock-j2jm6u4w.web.app

(expires Tue, 14 May 2024 15:42:20 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: dd76fe72958fe2910fef9d53f0b4539b82b849db

bobo-k2
bobo-k2 requested changes May 6, 2024
View reviewed changes
Copy link
Contributor

@bobo-k2 bobo-k2 left a comment

Choose a reason for hiding this comment

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

I would like to suggest a bit different approach which doesn't change existing codebase that much and it will be easier to clean up later when we stop supporting lockdrop accounts.

I don't think you need to modify dapp staking repository and service since logic you need is implemented in DappStakingServiceV2Ledger.
Just change useDappStaking to make call to a proper service based on isLockdropAccount
You will also need to introduce DappStakingServiceV2Ledger for EVM.

@impelcrypto
Copy link
Member Author

impelcrypto commented May 6, 2024 •
edited

Thanks, @bobo-k2
You are right. The logic can be simplified as you suggest. I've tested on Shibuya and it worked well.
Please review again.

bobo-k2
bobo-k2 approved these changes May 6, 2024
View reviewed changes
Copy link
Contributor

@bobo-k2 bobo-k2 left a comment

Choose a reason for hiding this comment

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

LGTM

gtg7784
gtg7784 reviewed May 7, 2024
View reviewed changes
src/staking-v3/hooks/useDappStaking.ts Outdated
useChainMetadata,
useNetworkInfo,
useLedger,
ETHEREUM_EXTENSION,
Copy link
Member

Choose a reason for hiding this comment

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

I think exporting constant in hooks is not a good way, I think moving ETHEREUM_EXTENSION to another files like src/constants/~~ but I want to know what you thinking about it

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. Please refer to this commit.

@gtg7784 gtg7784 self-requested a review May 8, 2024 03:01
gtg7784
gtg7784 approved these changes May 8, 2024
View reviewed changes
Copy link
Member

@gtg7784 gtg7784 left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@impelcrypto impelcrypto merged commit 3b32d0d into release-hotfix May 8, 2024
5 of 6 checks passed
@impelcrypto impelcrypto deleted the fix/lockdrop-unlock branch May 8, 2024 12:14
@impelcrypto impelcrypto mentioned this pull request May 8, 2024
Merged
6 tasks
impelcrypto added a commit that referenced this pull request May 14, 2024
@impelcrypto @gtg7784 @bobo-k2
hotfix: merge the hotfix to main branch (#1281)
7b0f081 
* add native bridge deposit warning banner (#1275)

* add native bridge deposit warning banner

* update warning text on native bridge

* disable desposit button on bridge

* update i18n warning variable naming

* Revert "add native bridge deposit warning banner (#1275)" (#1276)

* fix: enables unlock and withdrawal from Lockdrop accounts (#1279)

* feat: updated unlock logic

* fix: updated dApp staking logic for Lockdrop accounts

* Revert "fix: updated dApp staking logic for Lockdrop accounts"

This reverts commit 0c00f9c.

* Revert "feat: updated unlock logic"

This reverts commit 4a38747.

* fix: simlifize the logic

* fix: moved variable for ETHEREUM_EXTENSION

* update withdrawal warning 20h -> 5h (#1284)

* fix: updating lockdrop contract address for Shiden (#1285)

---------

Co-authored-by: Alan <tae.gun7784@gmail.com>
Co-authored-by: Bobo <bobo.kovacevic@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bobo-k2 bobo-k2 bobo-k2 approved these changes

@gtg7784 gtg7784 gtg7784 approved these changes

@codespool codespool Awaiting requested review from codespool

@ayumitk ayumitk Awaiting requested review from ayumitk

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@impelcrypto @bobo-k2 @gtg7784