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

Null Wallet Indexer support in NEP6Wallet (for NEO 2.x) #961

Merged
merged 4 commits into from
Jul 28, 2019
Merged

Null Wallet Indexer support in NEP6Wallet (for NEO 2.x) #961

merged 4 commits into from
Jul 28, 2019

Conversation

devhawk
Copy link
Contributor

@devhawk devhawk commented Jul 26, 2019

neo-express uses a custom Wallet subclass to store all of the wallets (consensus node and standard account) for a given developer blockchain instance in a single file without encryption. neo-express has export functionality to enable developer to convert these developer blockchain wallets to NEP-6 standard wallets.

NEP6Wallet's constructor takes a WalletIndexer argument, which needs to access the Blockchain.Singleton instance. When neo-express is exporting wallets, there is no running blockchain instance, so the WalletIndexer hangs attempting to access Blockchain.Singleton.

This PR enables NEP6Wallet to accept a null WalletIndexer instance. Wallet functionality that depends on the indexer, such as WalletHeight property, GetCoins and GetTransactions, have been updated to return default values when the indexer is null.

This change will not affect existing NEP6Wallet code paths as those code paths always pass in a valid WalletIndexer instance.

Note, this change is for the NEO 2.x branch. The NEO 3.x branch will not need this change due to the change from UTXO model to Native Contracts for NEO/GAS tokens.

Here is the wallet export code from neo-express that demonstrates the use of the null WalletIndexer parameter.

public void Export(string filename, string password)
{
    var nep6Wallet = new Neo.Wallets.NEP6.NEP6Wallet(null, filename, Name);
    nep6Wallet.Unlock(password);
    foreach (var account in GetAccounts())
    {
        nep6Wallet.CreateAccount(account.Contract, account.GetKey());
    }
    nep6Wallet.Save();
}

@devhawk devhawk changed the title Null Wallet Indexer support in NEP6Wallet Null Wallet Indexer support in NEP6Wallet (for NEO 2.x) Jul 26, 2019
foreach (UInt256 hash in indexer.GetTransactions(accounts.Keys))
yield return hash;
lock (unconfirmed)
if (indexer != null)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note, GitHub's diff of GetTransactions is a little weird. Since GetTransactions is using yield return, I wrapped the existing function in an indexer null check. So if the indexer is null, this function implicitly returns an empty UInt256 enumerable.

@shargon
Copy link
Member

shargon commented Jul 26, 2019

I had the same problem in the past, so thanks!

Note, this change is for the NEO 2.x branch. The NEO 3.x branch will not need this change due to the change from UTXO model to Native Contracts for NEO/GAS tokens.

Is this really needed en neo2?

@lock9
Copy link
Contributor

lock9 commented Jul 26, 2019

@shargon this is needed for his development tools. I will ask more explanations to him, but it seems this is mandatory.
I think this won't change how things in production, except if people were working using "try catch" statements.

@vncoelho
Copy link
Member

vncoelho commented Jul 27, 2019

Harrryy Harryyy, I do not know, this make us move slower...but faster at the same time.

Lets focus on Neo3.

It is up to @erikzhang, testing Neo2 LTS with new feature consumes part of the time we could dedicate to the next major release.

@shargon shargon requested a review from erikzhang July 27, 2019 08:05
@devhawk
Copy link
Contributor Author

devhawk commented Jul 27, 2019

@vncoelho my focus is on developer experience and tools, which are needed for NEO2 and NEO3.

Due to the WalletIndexer, NEP6Wallet is tightly coupled to a running instance of the blockchain. neo-express doesn't have a running blockchain instance during export scenarios. So my alternatives to support wallet export are either this minor change to NEP6Wallet or to replicate the entire NEP6Wallet.Save code path in neo-express. I rather not replicate an existing code path that has been running in production for years if I can avoid it.

@vncoelho
Copy link
Member

vncoelho commented Jul 27, 2019

I agree Harry, we just need a syncronization for making these minor changes without any worry.

@vncoelho vncoelho merged commit 65fe1b2 into neo-project:master-2.x Jul 28, 2019
@devhawk devhawk deleted the devhawk/null-wallet-indexer-2 branch July 30, 2019 20:09
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.

5 participants