Skip to content
This repository has been archived by the owner on Dec 7, 2023. It is now read-only.

Add cosigner #545

Closed
wants to merge 5 commits into from
Closed

Add cosigner #545

wants to merge 5 commits into from

Conversation

doubiliu
Copy link

Currently,in OnInvokeCommand,we use new cosigner[0] as a inital paramter.This will caluse falut in CheckWitness.

We need to add the relevant cosigner

@@ -282,6 +282,16 @@ private bool OnInvokeCommand(string[] args)
{
var scriptHash = UInt160.Parse(args[1]);

List<Cosigner> signCollection = new List<Cosigner>();
Copy link
Member

Choose a reason for hiding this comment

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

We should be able to choose wich cosigner we want, imagine a wallet with 1.000 of address, we can't add all of them

Copy link
Author

Choose a reason for hiding this comment

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

mmm.I have thought about this.But generally only those exchanges will create a lot of address and they will not use this node to invoke command.
@superboyiii Can you help explain it together?

Copy link
Member

Choose a reason for hiding this comment

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

Then with only the first one with balance it's enough?

Copy link
Author

Choose a reason for hiding this comment

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

So we need to use manual witness first to make the pre-execution pass, calculate the contract's execution consumption, and then choose an account with a sufficient amount from the available accounts to pay the fees. Is this equal to implementing the logic about UTXO in NEO 2 again?

Copy link
Member

Choose a reason for hiding this comment

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

I think that the user should specify which account and with which scope want to sign

Copy link
Member

Choose a reason for hiding this comment

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

Then with only the first one with balance it's enough?

Take first one as default is dangerous,
If the contract is not source-open, it can steal asset from user easily.

I vote for the user can manually specify which account and with which scope.
But we need a method to make it easy for using

Copy link
Author

Choose a reason for hiding this comment

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

Great proposal.I think I can implement the specified account first, the scope is a bit complicated, and in general, the scope of this invoke script should be fixed

Copy link
Member

Choose a reason for hiding this comment

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

Maybe duplicating the methods it's easier InvokeGlobal, InvokeCalledByEntry...

Copy link
Author

Choose a reason for hiding this comment

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

I think CalledByEntry is enough.It just a little script to invoke contract function.No other script will call this script.

Copy link
Member

Choose a reason for hiding this comment

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

I agree @shargon , we should be able to choose co-signer.

Nowadays on Neocompiler.io we have this feature for invoking.

image

We are adjusting for neo3 within these next days.

Comment on lines 299 to 303
if (specifyAddress.Length != signCollection.Count)
{
Console.WriteLine("Error: Invalid address");
return true;
}
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't make sense since if I input invoke 0x0a49cc60d53a41fcf42f34632baa6ca64f987c87 deploy NPvKVTGZapmFWABLsyvfreuqn73jCjJtN1, it'll surely fail here since specifyAddress.Length will be 2.

Comment on lines 284 to 285
var specifyAddress = new String[args.Length - 2];
Array.ConstrainedCopy(args, 2, specifyAddress, 0, args.Length - 2);
Copy link
Member

Choose a reason for hiding this comment

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

It's a hard code. I think var specifyAddress = args.Last(); is more reasonable.

List<Cosigner> signCollection = new List<Cosigner>();
using (SnapshotView snapshot = Blockchain.Singleton.GetSnapshot())
{
UInt160[] accounts = CurrentWallet.GetAccounts().Where(p => !p.Lock && !p.WatchOnly).Select(p => p.ScriptHash).Where(p => NativeContract.GAS.BalanceOf(snapshot, p).Sign > 0).ToArray();
Copy link
Member

Choose a reason for hiding this comment

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

Open wallet isn't necessary for invoking, this will cause exception.

Fix
@superboyiii
Copy link
Member

I'll close this.

@superboyiii superboyiii closed this Apr 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants