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

Added functionality for multiple wallets #243

Merged
merged 24 commits into from
Aug 2, 2022

Conversation

ismail9001
Copy link
Contributor

  • Storing by address
  • Fetching list of wallets
  • Deleting by address

@DarthMike
Copy link
Member

Hello @ismail9001 what's the use-case for this change? Not sure if needed in the library

@ismail9001
Copy link
Contributor Author

Hello @DarthMike!
Unfortunately, your library is limited to working with only one wallet.
In my case, the library did not allow me to get a list of previously created accounts without duplicating the stored password information and address lists in the additional storage.

I think these features will add popularity to your library

@DarthMike
Copy link
Member

@ismail9001 OK makes sense. Not sure if this exact implementation woud be the best, but need to think about it (i.e. always assuming more than one account). I'll post review comments.

@ismail9001
Copy link
Contributor Author

Thank you, I'll be waiting

web3swift/src/Account/EthereumKeyStorage.swift Outdated Show resolved Hide resolved
web3swift/src/Account/EthereumKeyStorage.swift Outdated Show resolved Hide resolved
web3swift/src/Account/EthereumKeyStorage.swift Outdated Show resolved Hide resolved
web3swift/src/Account/EthereumKeyStorage.swift Outdated Show resolved Hide resolved
web3swift/src/Account/EthereumAccount.swift Outdated Show resolved Hide resolved
web3swift/src/Account/EthereumAccount.swift Outdated Show resolved Hide resolved
web3swift/src/Account/EthereumKeyStorage.swift Outdated Show resolved Hide resolved
@DarthMike
Copy link
Member

@ismail9001 Just reviewed the code, I think APIs would be better if we change them, see the comments in code.

@ismail9001 ismail9001 requested a review from DarthMike July 5, 2022 13:53
Copy link
Member

@DarthMike DarthMike left a comment

Choose a reason for hiding this comment

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

Thanks now it's looking good

@DarthMike DarthMike changed the base branch from master to develop July 8, 2022 09:51
@DarthMike
Copy link
Member

@ismail9001 Looks good, but we need to do PR against develop. After conflicts solved we'll run the tests in CI

# Conflicts:
#	web3sTests/Account/EthereumKeyStorageTests.swift
#	web3sTests/Utils/KeyUtilTests.swift
#	web3swift/src/Account/EthereumAccount.swift
#	web3swift/src/Account/EthereumKeyStorage.swift
@ismail9001
Copy link
Contributor Author

I changed destination of PR to develop

@DarthMike
Copy link
Member

@ismail9001 test failing when deleting keys in Linux
Test Case 'EthereumKeyStorageTests.testDeleteAllPrivateKeys' started at 2022-07-11 15:13:14.682 Could not delete address: Error Domain=NSCocoaErrorDomain Code=4 "The file doesn’t exist." Could not delete addresses: failedToDelete /__w/web3.swift/web3.swift/web3sTests/Account/EthereumKeyStorageTests.swift:86: error: EthereumKeyStorageTests.testDeleteAllPrivateKeys : failed - Failed to delete all private keys: failedToDelete

@ismail9001
Copy link
Contributor Author

I added an address output to check the reason for the test crash. Please run the test on linux

@ismail9001
Copy link
Contributor Author

Please run tests again

@ismail9001
Copy link
Contributor Author

On Linux was strange error

/__w/web3.swift/web3.swift/web3sTests/ERC721/ERC721Tests.swift:150: error: ERC721MetadataTests.test_ReturnsMetatada : failed - Expected tokenMetadata but failed dataCorrupted(Swift.DecodingError.Context(codingPath: [], debugDescription: "The given data was not valid JSON.", underlyingError: Optional(Foundation.JSONError.unexpectedCharacter(ascii: 60, characterIndex: 0)))).

but I didnt change this functionality

@ismail9001
Copy link
Contributor Author

I tried to find out the cause of the problem, but only managed to find out that the test falls on the line

var metadata = try JSONDecoder().decode(Token.self, from: data) in ERC721Metadata class

what is the problem of the linux test, unfortunately, I cannot say

@DarthMike
Copy link
Member

It's passing now, so will merge

@DarthMike DarthMike merged commit d2bc38d into argentlabs:develop Aug 2, 2022
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.

3 participants