Neo Shell extension model #4
Conversation
…et to neo-worknet
…can handle transactions Still WIP
| { | ||
| public bool TryFindCommand(string[] args, out ShellExtension? command) | ||
| { | ||
| foreach (string arg in args) |
There was a problem hiding this comment.
seems odd that we are searching all the args instead of just the first one.
There was a problem hiding this comment.
I was thinking potential non-positional parameters is it possible that we have "neosh --json worknet ..." ?
There was a problem hiding this comment.
it feels like that might lead to argument collision. For example, If I created a "transfer" extension, how would shell differentiate between that extension and the NEP11 extension transfer command? (assuming both installed).
Requiring a unique initial position argument that identifies the extensions seems like a reasonable constraint.
| return process; | ||
| } | ||
|
|
||
| private (string, int) ExtractCommandFromArguments(string[] args, string command) |
There was a problem hiding this comment.
This is a nit pick, but did you do this?
| public override Version? Version => null; | ||
|
|
||
| public DevWallet(ProtocolSettings settings, string name, IEnumerable<DevWalletAccount>? accounts = null) : base(string.Empty, settings) | ||
| class DevWallet : Wallet |
There was a problem hiding this comment.
BCTK Lib has a ToolkitWallet class you could use instead of DevWallet. I plan to move express over to ToolkitWallet at some point in the future
There was a problem hiding this comment.
ok. This probably needs to be part of another refactoring/cleanup for the shell. The model was copied from express along with other stuff like DevWalletAccount.cs etc.
There was a problem hiding this comment.
I'm OK with doing this as part of a general cleanup pass. Please open an issue to track and include any other areas of the code you think need to be addressed. I have thoughts as well, but I would like to hear your thoughts on eliminating technical debt here.
|
Note, PR is failing to pack. The path to the package logo is specified in src\Directory.Build.props, but because I asked to move the NEP11 extension down a level in the folder hierarchy, that project can't locate the logo. |
|
Great work on the code review feedback. Once you get the build fixed and the refactor/cleanup issue filed, I think we're ready to merge |
Hmm, do we have to refer to the logo conditionally? It's Does it mean for shell-ext/nft we will have to go two levels up? |
The first version of the neoshell extension mode.