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

MultiSigInbox Plugin #487

Closed
wants to merge 29 commits into from
Closed

MultiSigInbox Plugin #487

wants to merge 29 commits into from

Conversation

shargon
Copy link
Member

@shargon shargon commented Jan 27, 2021

vncoelho
vncoelho reviewed Jan 27, 2021
View reviewed changes
Copy link
Member

@vncoelho vncoelho left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks very good @shargon.

I like the name you gave to the plugin.
I think this will be very useful to the ecosystem.

vncoelho
vncoelho reviewed Jan 28, 2021
View reviewed changes
src/MultiSigInbox/MultiSigInboxPlugin.cs Outdated
}

[ConsoleCommand("inbox list", Category = "MultiSigInbox", Description = "List pending transactions")]
private void OnInboxList(bool pending = true)
Copy link
Member

Choose a reason for hiding this comment

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

@shargon, I think that signers can send both a complete transaction that is partially signed or just the hash and a partial signature.
In this way we avoid unnecessary traffic.

Copy link
Member

Choose a reason for hiding this comment

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

However, transactions are usually small and not really a problem for the P2P.
Perhaps all should send the complete tx.

@superboyiii superboyiii mentioned this pull request Feb 11, 2021
Closed
27 tasks
@superboyiii
Copy link
Member

Let's move on? I think it's useful although it's too late for RC1 but maybe we could implement it in RC2.

@shargon
Copy link
Member Author

shargon commented Mar 1, 2021

The plugin only could be used by white listed wallets, it's this ok?

@superboyiii
Copy link
Member

The plugin only could be used by white listed wallets, it's this ok?

I think it's enough.

@shargon shargon mentioned this pull request Mar 2, 2021
Merged
@shargon
Copy link
Member Author

shargon commented Mar 8, 2021

@neo-project/ngd-shanghai could you help me with testing?

@shargon shargon marked this pull request as ready for review March 8, 2021 08:15
@superboyiii
Copy link
Member

@neo-project/ngd-shanghai could you help me with testing?

Sure.

@erikzhang
Copy link
Member

The code looks good. Please wait for the test.

ZhangTao1596
ZhangTao1596 reviewed Mar 9, 2021
View reviewed changes
src/MultiSigInbox/MultiSigInboxPlugin.cs Outdated
{
public class MultiSigInboxPlugin : Plugin, IP2PPlugin
{
public const string StatePayloadCategory = "MultiSignatureInbox";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public const string StatePayloadCategory = "MultiSignatureInbox";
public const string MutiSigPayloadCategory = "MultiSignatureInbox";

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@superboyiii
Copy link
Member

superboyiii commented Mar 9, 2021 •
edited
Loading

image
@shargon

@shargon
Copy link
Member Author

shargon commented Mar 9, 2021 •
edited
Loading

'json with single quote'

image

could you copy here your input as a text?

@superboyiii
Copy link
Member

superboyiii commented Mar 9, 2021 •
edited
Loading

'json with single quote'

image

could you copy here your input as a text?

{"type":"Neo.Network.P2P.Payloads.Transaction","hex":"ALgo/ma0i5cAAAAAAKzjMwAAAAAA4hkAAAFsilPbqaK7CHEcRDMlSXnkU977vAEAWAsAZAwUzSoUqGD8JHFC\u002BkCpIXAF1Vus5dUMFGyKU9uporsIcRxEMyVJeeRT3vu8FMAfDAh0cmFuc2ZlcgwU9WPqQLwoPU0OBcSOowWz8qBzQO9BYn1bUjk=","items":{"0xbcfbde53e479492533441c7108bba2a9db538a6c":{"script":"EwwhAi/GL/XYE162iLZIO/ZV4Hqpy2og4wxdrfwJurDRI1UbDCEDZ\u002BCfDFJRx87OMc90EAM3pgj4T42FoVjyZ//EKjxYZxsMIQOoSJI0seBp53388sysHMFVPC1acgPH31ydUT8GIQmpyhNBe85spQ==","parameters":[{"type":"Signature"},{"type":"Signature"},{"type":"Signature"}],"signatures":{"022fc62ff5d8135eb688b6483bf655e07aa9cb6a20e30c5dadfc09bab0d123551b":"lK1ZGUWJYqAiCYdA2NZeyY7zAeUGtDze0nfaoQn//Z1djQk3FEv6oh7Ha5aRpj85/YYHBsrdN1jDzdcHKf4AuQ=="}}}}

Looks like it happened when write(witness) because incomplete context will not GetWitness().
image

@roman-khimov
Copy link
Contributor

Do we need this if we can have more generic Notary subsystem (neo-project/neo#1573)? This only works for a whitelist of addresses while Notary subsystem is available for anyone and can handle a lot more cases. It also adds another proper paid-for service for the network which can be beneficial for project economics. Notary subsystem has complete implementation in neo-go and it's already integrated into NeoFS (nspcc-dev/neofs-contract#51), it works.

@shargon
Copy link
Member Author

shargon commented Mar 10, 2021

@superboyiii could you check it again please

@superboyiii
Copy link
Member

image
@shargon inbox sign failed.

superboyiii and others added 2 commits March 11, 2021 16:48
@superboyiii
Merge branch 'master' into multisig-inbox
a83dcd3
@shargon @superboyiii
Update src/MultiSigInbox/MultiSigInboxPlugin.cs
fc17e5b 
Co-authored-by: Owen Zhang <38493437+superboyiii@users.noreply.github.com>
@shargon
Copy link
Member Author

shargon commented Mar 11, 2021

@superboyiii please copy your input as text here

@superboyiii
Copy link
Member

@superboyiii please copy your input as text here

neo> inbox list
Transaction hashes
0x6cfe4b9371b075fbad4508b44c3b59fab48160d180195ce13bca82c6c3b98c02
0xdb2e8d4795b1b3bb966fc85ac2272660efe299e4a4763ebfc6e819f05dfdde1e
0x6b1baa343dd253c35d53017d20f62ea8a063f8ef9fd55457915c1b27de63ec56
0x0480f5fdbcce18ed77f69b0e33790d51eb7c2441f64986b807f50ab9d677eabf
neo> inbox read 0x6cfe4b9371b075fbad4508b44c3b59fab48160d180195ce13bca82c6c3b98c02
{"type":"Neo.Network.P2P.Payloads.Transaction","hex":"AJApK2\u002B0i5cAAAAAAJwXdAAAAAAA7S4AAAH4JGXeIikja1DoxoxIKrkYu6bW7AEAWAsAZAwUXy/DPyeOyMAOrQfCSWlLdF\u002BI4E4MFPgkZd4iKSNrUOjGjEgquRi7ptbsFMAfDAh0cmFuc2ZlcgwU9WPqQLwoPU0OBcSOowWz8qBzQO9BYn1bUjk=","items":{}}
neo> inbox sign 0x6cfe4b9371b075fbad4508b44c3b59fab48160d180195ce13bca82c6c3b98c02
error: One of the identified items was in an invalid format.

You could try it in my tempo testnet.
https://github.com/superboyiii/neo-node/tree/RC1-CI01238

@shargon
Copy link
Member Author

shargon commented Mar 11, 2021

@superboyiii Thanks, please test it

@ZhangTao1596 ZhangTao1596 mentioned this pull request Mar 24, 2021
Open
@roman-khimov roman-khimov mentioned this pull request Apr 11, 2021
Closed
@vncoelho
Copy link
Member

vncoelho commented Sep 23, 2021 •
edited
Loading

Maybe we could advance on this, could be a good feature besides Neo Committee that is now consolidated.

@vncoelho
Copy link
Member

I still think this is a good plugin, @shargon.

Specific nodes can use it if they want.
If it is optional I do not see a problem.

@roman-khimov
Copy link
Contributor

It's still too limited (hi, #487 (comment)), extensible messages can be sent by a very limited set of addresses.

@Jim8y Jim8y added the Need Active Pr will be closed after one week if no new activity. label Feb 12, 2024
@Jim8y
Copy link
Contributor

Jim8y commented Feb 17, 2024

close as inactive

@Jim8y Jim8y closed this Feb 17, 2024
@shargon shargon deleted the multisig-inbox branch February 17, 2024 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vncoelho vncoelho vncoelho left review comments

@ZhangTao1596 ZhangTao1596 ZhangTao1596 left review comments

@superboyiii superboyiii superboyiii left review comments

Assignees
No one assigned
Labels
Need Active Pr will be closed after one week if no new activity.
Projects
None yet
Milestone
No milestone
7 participants
@shargon @superboyiii @erikzhang @roman-khimov @vncoelho @Jim8y @ZhangTao1596