-
Notifications
You must be signed in to change notification settings - Fork 285
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
fix(core-api): prevent cold wallet response #1955
fix(core-api): prevent cold wallet response #1955
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another solution I would prefer is to simply check wether the wallet manager has a specific address before trying to get one.
The PoolWalletManager
has an exists
function for exactly that purpose:
https://github.com/ArkEcosystem/core/blob/develop/packages/core-transaction-pool/src/pool-wallet-manager.ts#L45
Though the comment seems to be wrong, since the WalletManager has no such function.
So...
- Move
exists
fromPoolWalletManager
toWalletManager
- Add
exists
check here and return null otherwise:
https://github.com/ArkEcosystem/core/blob/develop/packages/core-api/src/repositories/transactions.ts#L486
This way there's no need to change anything in the endpoint methods as they can
trust what the repository returns.
@supaiku0 Thanks for the feedback. I've done so and my editor is telling me that the function exists doesn't exist on type WalletManager. Do you know where I should add a typing for this? |
Codecov Report
@@ Coverage Diff @@
## develop #1955 +/- ##
==========================================
- Coverage 38.61% 38.6% -0.01%
==========================================
Files 354 354
Lines 7762 7764 +2
Branches 1143 1172 +29
==========================================
Hits 2997 2997
- Misses 4751 4753 +2
Partials 14 14
Continue to review full report at Codecov.
|
The method needs to be removed here https://github.com/ArkEcosystem/core/blob/develop/packages/core-transaction-pool/src/pool-wallet-manager.ts#L45 as the pool manager extends the wallet manager. |
d808392
to
3481641
Compare
ill take care of the extra commits 😓 |
0bd05d3
to
2c255a3
Compare
3481641
to
b4a47dd
Compare
https://circleci.com/gh/ArkEcosystem/core/10033
|
@supaiku0 |
Ref: #1949
Proposed changes
I will test if the bug is also present on v2 today, I would like to get a review on whether this makes sense. All tests were done incrementally, initially the response (without account.publicKey === null) check was that of an empty cold wallet; now it simply returns an error "Account not found" (as wanted).
Types of changes
Checklist