Conversation
|
NEP-7 mentions backwards compatibility in the sense that if a contract does not respond correctly to the |
|
I agree with @dicarlo2. Also, this is a very large change. A change such as this should have tests. Tests to show what happens with old contracts. Tests with new contracts that respond to |
|
@dicarlo2 Yes, since most people return @erikzhang The current implementation now invokes a set of I don't really see the upside of triggering multiple times, and the downside is that it is harder to count the total output as there is no assurance of a single trigger per transaction. Could you explain the use case for that? |
|
@erikzhang A possible modification if we invoke once per Edit: Sorry, this doesn't really make sense for |
|
@localhuman the change is not meant to be backward compatible with old contracts, as mentioned in the NEP. I'm just saying we should push forward with this, if this is ever going to be done, such that there is less incompatible contracts out there. |
|
Are you saying that if we rebuild the chain with these changes, we will get different results than what is currently there? If that is the case, I think its foolish. But either way there should be tests to show how and where things will break. |
| @@ -3,6 +3,8 @@ | |||
| public enum TriggerType : byte | |||
| { | |||
There was a problem hiding this comment.
Would it be better for this to be an enum style bit mask?
That way if an existing contract checks for Verification, even if the contract is VerificationR it will match.
There was a problem hiding this comment.
Hmm, since Verification is typically just a CheckWitness of the contract owner, and this is a receive rather than a send, existing contracts will just fail too with false.
|
@localhuman no, we will need a way to prevent that from happening. I don't see a clean way of doing it though, besides activating by blockheight - any suggestions? Edit: by the way, I didn't see any test harness for running contracts either - where do you suggest doing it? |
|
Another way to completely preserve backward compatibility would be to add a new |
AshRolls
left a comment
There was a problem hiding this comment.
As per @localhuman comments this PR needs more thought before we can merge.
|
I think NEP-7 shouldn't exist at all. What is the case that this allow that cannot be done in current system in two transactions ? Breaking changes in every single contract should have a very strong motivation, this is not one. |
|
One example is a contract that does not allow any deposit, and so guards against any accidental sends to it. Once again, by adding a contract property, we can prevent all old contracts from breaking. So I think it is possible to move forward in a safe way. Please take note that this nep has nothing to do with withdrawals from a contract. So two txns or not is not relevant here. |
|
@RavenXce you can reject the transaction forcing verification to run today, Narrative ICO and Bridge ICO did that already and any project that requires the feature can do the same. You can do it in Switcheo platform. The only guard is for accidental sends away from any reasonable interface. No need for breaking changes. Besides breaking changes, there is zero tests - both passing and expected failures. |
|
@canesin that is indeed correct, assuming the client does the correct things. We have already implemented that awhile back: https://github.com/ConjurTech/switcheo-token/blob/master/Switcheo_Token/Switcheo_Token.cs#L46 However, I feel that this is a useful change, and is akin to the I feel this will be a massive improvement to the current two triggers, which is hard to get right due to the mixing of concerns. If you look at the code snippet I posted above, I'm sure you can see that it is extremely hard to reason about the different possible cases. To reiterate, I've suggested a way to deviate from the original proposal such that the change is non-breaking. Regarding tests, I don't see any real tests for any case in any platform currently (unless I'm mistaken). If someone would give me some direction, I would be most glad to add them - my first thought when implementing this was actually to do so! Starting this endeavor would be good for the platform as a whole anyway. Anyway, since this NEP was proposed by @erikzhang, I will leave it up to him to decide if he wishes to close it, and will wait for some consensus before proceeding with further changes. |
|
I don't wish to be argumentative, but I would like to bring up another example, which I just remembered. Here I have to guard against double counting of received assets, bloating up the code, and storing a value that is only used once: Overall, the whole counting of assets issue is a very easily missed attack vector, and there should probably be a proper, sane way to do it? |
The results will not be different. Because |
You are right. One contract in each transaction should be called only once. But we must make sure that I will do the changes. |
|
And we need to test with old contracts |
|
Okey, let's temporarily disable this feature on the mainnet and deploy it on testnet first. |
|
@erikzhang can't we treat this with more specificity ? For example: "It will be deployed in testnet on block X and if successful on mainnet on block Y." If you can't give block numbers, dates and times. |
Since this stops relaying transactions this needs to be gated by block height so as to not block currently running ICOs. If it’s not gated by block height in the code people will deploy at various different times and break investing in various ICOs that don’t implement VerificationR. I want to run with other improvements on mainnet, so I’ve just disabled VerificationR and ContractPayable checks while running with other improvements. |
|
Agreed, this ( in conjunction with the Contract Doing a check by block height is not ideal, as this would not translate to private/test other networks. Having a |
|
I think @erikzhang already pointed out rebuilding the chain doesn’t have issues since the methods only run during consensus, not when resync of the chain. I get your desire though to not stop existing contracts from working by versioning the contracts. Seems like a good idea to me and does require someone to take up the work for more changes to handle contract versioning, etc. |
All contracts are executed by all nodes when you get the blocks from the begining, so the result must be the same, otherwise if you start without chain, and get the blocks from others nodes, your storages could be different with the others. We need to ensure that the storages (and the results) was the same Consensus nodes only sign and validate the block, but all nodes need to execute all the transactions in his own chain, so if you change the logic, and old transactions now have a different value as before the change, we have a problem |
|
We need to check the |
|
I don't think it's necessary. |
|
After thinking about this some more (while implementing for neo-one's node), I'm starting to wonder if we actually need this functionality. At a high level, NEP-7 seems to attempt to solve two problems (based on this discussion and the proposal text):
Today, the problem is that smart contracts are valid addresses and thus can be used as the address of an output for any transaction, but can only properly react to transfers in an Overall, I'd like to revisit what we're trying to accomplish with this change. If my understanding above is correct, it seems like the simplest solution would be to just make it so that outputs cannot use a smart contract address unless they are an In summary, an alternative implementation that does not increase the API surface area could be to make the following changes:
The above changes would guarantee that a smart contract can both refuse and react to UTXO transfers. The change would be backwards compatible with the @erikzhang @localhuman @canesin @shargon @RavenXce thoughts? |
|
@dicarlo2 that is a great suggestion, it is a lot closer to @erikzhang original intent and design for the smart contract system, are you willing to do a NEP for that ? We could ask @shargon and @osmirnov to implement on the reference and move NEP-7 to "Replaced" status that is in NEP-1. I understand that hard work from @RavenXce went into this proposal implementation, but maybe we should have discussed the proposal in itself more. The only point that I think we should keep is the multiple invoke per transaction, maybe there could be some iteration on how to handle that better from the smart contract side. |
The trigger is only invoked once per receiving address (smart contract), regardless of outputs.
I think it reduces the API surface area as there is only one method used for receiving system assets.
I don't get how this matters? If the output is not a deployed contract, or does not comply to NEP-7, nothing actually happens - so it is the same as sending NEO to a random ICO that has ended currently.
The whole point is that the smart contract author can reject using
I think the issue with using a same trigger is that then there is no way to differentiate a trigger crafted through the invocation
If any |
Ah, I missed that.
What about a contract that needs to support multiple methods for receiving system assets? How are they supposed to use Similarly, even in the single method case, what about a contract that requires arguments to the method for receiving system assets? They again, have to track that there are outputs in the
The point of this is to enforce correctness at the blockchain level so that it's impossible to accidentally send system assets to a contract that does not react to them, replacing the need for extra triggers.
It does, but what is the advantage of that?
What is the advantage of doing this? Does this not also complicate smart contract authoring? If you wanted to take into account state changes from
Hence why I mentioned we could (and probably should) enforce that a smart contract is called exactly once. |
I think I'd prefer if we were more restrictive to start and then if there is a good use-case for multiple invokes for transactions that send system assets, then and only then do we consider the best way to implement it. |
|
At any rate, the current solution is problematic for (at least) two reasons:
Those 2 alone should be enough to consider alternatives. |
I actually agree it would be simpler for most cases if The way that I implemented it originally, But anyway, some sort of tracking has to be done if the SC author wants to use both triggers in tandem. This is no less complicated then how it is currently as double appcalls have to be tracked (regardless of how many operation types there are for receiving assets)
I do not get this point however. We are talking about the same transaction so they would have the same outputs?
I think if we want to keep the API surface low / client requirements simple, as well as reduce errors in sending system assets, this is helps a lot, isn't it? The guarantee as a smart contract author that as long as my contract receives assets, X will be triggered once and only once per txn, is a useful thing to have.
This is actually done (again?) by the |
Right, so I think we agree that, at least for multiple operations,
The current design also doesn't work at all for multiple app calls or multiple methods in the
What I meant by that is rather than being able to just stick all of your system asset handling code in
I think we agree on this point, see point 3 on implementation ("we could require that corresponding smart contracts are executed exactly once in the script"). I just wanted to thank @RavenXce for his comments - I think it has helped us all explore the problem a bit more thoroughly. Despite my mistake of thinking
My proposal is that we instead do the following (mostly copy-pasted from above):
Here's how it compares for the above issues:
It also contains all of the same properties of the current solution:
It also leaves the door open for allowing multiple invocations of the same smart contract when system assets are attached should we decide that it is necessary in the future. If we were to allow this, I believe any solution will require additional tracking, so perhaps that's something we could consider building into NEO directly - for example a special syscall to "mark" an output as consumed and another syscall to fetch all unmarked outputs. Still, whether it's built-in or smart contract authors just use storage to mark, we still have the guarantee that |
We can deploy NEP-7 with a block height for
We can provide a new API to get the number of times the current contract is invoked. |
|
@erikzhang what about the other 7 issues?
What if the transaction doesn't cause |
I think they are not NEP-7 issue in nature. |
|
@erikzhang Sure, but what exactly is the point of NEP-7 if it's not taking a holistic approach to solving the issues? We seem to be adding extra triggers and thus extra complexity without really solving anything. Besides, I'm not seeing how they are not directly related to NEP-7, all of the stated issues are a direct result of NEP-7 and how it's implemented. Can you explain how they are not NEP-7 issues in nature? |
|
NEP-7 just added two new triggers and didn't do anything else. If a contract does not support If a contract does not support Besides, I don't see any other new problems with NEP-7. If there is, then it must be a pre-existing problem. |
|
@erikzhang Right, these are existing problems, and we're adding new triggers that add to the complexity of smart contract authoring, node implementation and handling among others without really solving anything. Have you considered the alternative solution I posted above? It's concerning that despite opposition from many key stakeholders - multiple members of City of Zion and NEO Tracker and very little support in general, we're still moving forward with this implementation. |
|
I've written a new template which using NEP-7 for ICO: public class Contract1 : SmartContract
{
private static readonly byte[] owner = "ATrzHaicmhRj15C3Vv6e6gLfLqhSD2PtTr".ToScriptHash();
private static readonly byte[] neo_asset_id = { 155, 124, 255, 218, 166, 116, 190, 174, 15, 147, 14, 190, 96, 133, 175, 144, 147, 229, 254, 86, 179, 74, 92, 34, 12, 205, 207, 110, 252, 51, 111, 197 };
private const int ico_start_time = 1506787200;
private const int ico_end_time = 1538323200;
public static event Action<byte[], byte[], BigInteger> transfer;
public static object Main(string operation, object[] args)
{
switch (operation)
{
case "totalSupply":
return totalSupply();
case "name":
return "ICO_Template_NEP7";
case "symbol":
return "ITN";
case "decimals":
return 8;
case "balanceOf":
return balanceOf((byte[])args[0]);
case "transfer":
return _transfer(ExecutionEngine.CallingScriptHash, (byte[])args[0], (byte[])args[1], (BigInteger)args[2]);
case "receiving":
return receiving();
case "received":
return received();
case "withdraw":
return withdraw();
default:
throw new InvalidOperationException();
}
}
public static BigInteger totalSupply()
{
StorageMap storage = Storage.CurrentContext.CreateMap(Prefixes.Metadata);
return storage.Get("totalSupply").AsBigInteger();
}
public static BigInteger balanceOf(byte[] account)
{
if (account.Length != 20) throw new ArgumentException();
StorageMap storage = Storage.CurrentContext.CreateMap(Prefixes.Balance);
return storage.Get(account).AsBigInteger();
}
private static bool _transfer(byte[] caller, byte[] from, byte[] to, BigInteger amount)
{
if (from.Length != 20 || to.Length != 20) throw new ArgumentException();
if (amount < 0) throw new ArgumentOutOfRangeException();
StorageMap storage = Storage.CurrentContext.CreateMap(Prefixes.Balance);
BigInteger balance_from = storage.Get(from).AsBigInteger();
if (balance_from < amount) return false;
bool verified;
if (from == caller)
verified = true;
else
verified = Runtime.CheckWitness(from);
if (!verified) return false;
Contract contract_to = Blockchain.GetContract(to);
if (contract_to != null && !contract_to.IsPayable) return false;
if (from != to && amount != 0)
{
BigInteger balance_to = storage.Get(to).AsBigInteger();
if (balance_from == amount)
storage.Delete(from);
else
storage.Put(from, balance_from - amount);
storage.Put(to, balance_to + amount);
}
transfer(from, to, amount);
return true;
}
public static bool receiving()
{
if (Runtime.Trigger != TriggerType.VerificationR)
throw new InvalidOperationException();
uint now = Runtime.Time;
if (now < ico_start_time || now > ico_end_time)
return false;
return true;
}
public static bool received()
{
if (Runtime.Trigger != TriggerType.ApplicationR)
throw new InvalidOperationException();
uint now = Runtime.Time;
if (now < ico_start_time || now > ico_end_time)
throw new Exception();
Transaction tx = (Transaction)ExecutionEngine.ScriptContainer;
BigInteger amount = 0;
foreach (TransactionOutput output in tx.GetOutputs())
if (output.ScriptHash == ExecutionEngine.ExecutingScriptHash && output.AssetId == neo_asset_id)
amount += output.Value;
if (amount == 0) throw new Exception();
byte[] sender = null;
foreach (TransactionOutput reference in tx.GetReferences())
if (reference.AssetId == neo_asset_id)
{
sender = reference.ScriptHash;
break;
}
StorageMap storage_balance = Storage.CurrentContext.CreateMap(Prefixes.Balance);
BigInteger balance = storage_balance.Get(sender).AsBigInteger();
storage_balance.Put(sender, balance + amount);
StorageMap storage_meta = Storage.CurrentContext.CreateMap(Prefixes.Metadata);
BigInteger totalSupply = storage_meta.Get("totalSupply").AsBigInteger();
storage_meta.Put("totalSupply", totalSupply + amount);
transfer(null, sender, amount);
return true;
}
public static bool withdraw()
{
if (Runtime.Trigger != TriggerType.Verification)
throw new InvalidOperationException();
return Runtime.CheckWitness(owner);
}
} |
I've done an implementation of NEP-7 since it seems to be stuck for awhile:
https://github.com/neo-project/proposals/pull/15/files
Think this should be done earlier rather than later so later contracts don't break. Right now most contracts are just ICO contracts so breakage is minimal.
I just did this in the most straightforward way possible, please advice if there is a better way.