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

Added new syscalls - GetInvocationCounter and GetNotifications #95

Merged
merged 16 commits into from
Sep 1, 2019

Conversation

igormcoelho
Copy link
Contributor

@belane you will need this for NEP-5

@igormcoelho
Copy link
Contributor Author

igormcoelho commented Aug 28, 2019

Can you please try to use it @belane? To see if it's responding correctly... like first invocation of kind.. second invocation.. etc. "mint" should be done only once, to simplify implementation.

@igormcoelho
Copy link
Contributor Author

igormcoelho commented Aug 28, 2019

Ah, it's breaking by format now... could you run a dotnet format for us @belane? when you have time please, during your tests.

P.S.: that's the first proof that the travis is working well to keep formats well-done 💪

@shargon
Copy link
Member

shargon commented Aug 28, 2019

Also, could you add the notifications?

@shargon
Copy link
Member

shargon commented Aug 28, 2019

GetNotifications was added, but require to be tested because i am not sure if it works xD

https://github.com/neo-project/neo/blob/c04ee5d277f522eb42bc8fc6a792b0aedf6ab0fc/neo/SmartContract/InteropService.cs#L237

@shargon shargon changed the title GetInvocationCounter Add new syscalls - GetInvocationCounter and GetNotifications Aug 28, 2019
@shargon shargon changed the title Add new syscalls - GetInvocationCounter and GetNotifications Added new syscalls - GetInvocationCounter and GetNotifications Aug 28, 2019
@igormcoelho
Copy link
Contributor Author

On GetNotifications, best solution now, in my opinion, is to return an object[], where first element is scripthash, and second is state. This corresponds to the internals used right now. A better solution is, in the future, create an Interop object just for notifications, so we can improve devpack a little bit more.

@@ -0,0 +1,8 @@
namespace Neo.SmartContract.Framework.Services.Neo
{
public class Notification : IScriptContainer
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this works, and it doesn't makes sense to inherit from IScriptContainer, bacause Notification is not a ScriptContainer.

Copy link
Contributor Author

@igormcoelho igormcoelho left a comment

Choose a reason for hiding this comment

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

Need to adjust into object[] pair.

@erikzhang
Copy link
Member

@igormcoelho I think we can revert the last commit. I believe that returning Notifications is also working. Let's test it.

@igormcoelho
Copy link
Contributor Author

igormcoelho commented Aug 29, 2019

Erik, I agree it could work, but it's a very unsafe side effect to rely on. If we want Notification to have its own life, we should create an Interop object for it, not treat it as an Array. At best scenario, it would be a Struct, not an Array... if that's intended perhaps we can create a Struct stack item on Notifications push, that's slightly better.... I mean, if it's worth risking, let's do it

// as struct
engine.CurrentContext.EvaluationStack.Push(notifications.Select(u => new VM.Types.Struct(new StackItem[] { u.ScriptHash.ToArray(), u.State })).ToArray());
            
// instead of array
engine.CurrentContext.EvaluationStack.Push(notifications.Select(u => new VM.Types.Array(new StackItem[] { u.ScriptHash.ToArray(), u.State })).ToArray());
            

For me it's good to revert, I'm just saying, it's not 100% correct in my opinion.

@erikzhang
Copy link
Member

What risk do you mean?

We convert class and array into Array in NeoVM, and convert struct into Struct.

@igormcoelho
Copy link
Contributor Author

igormcoelho commented Aug 29, 2019

We convert class and array into Array in NeoVM

I never thought this was official (for class), but a side effect. I always thought of class and struct to be equivalent to Struct. In this case I'll revert the commit.

@shargon
Copy link
Member

shargon commented Aug 29, 2019

We need to deal with the class, because is easier for devs.

@shargon
Copy link
Member

shargon commented Aug 29, 2019

Maybe need to be a struct?

@igormcoelho
Copy link
Contributor Author

@belane please confirm that this works for NEP-5 template... as Erik said, if class and array are equivalent for NeoVM, this should be fine.

@shargon
Copy link
Member

shargon commented Aug 29, 2019

I will make some unit test for both syscalls

Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

@erikzhang, it is quite confusing like this.

We used to consider that struct is also a class when programming in C++. How is it in C#?

Perhaps we could remove the struct and then consider everything as an array

@shargon
Copy link
Member

shargon commented Aug 29, 2019

Added unit test, it not works :( but you can play with it xD
@lightszero any idea ?

@shargon
Copy link
Member

shargon commented Aug 29, 2019

The problem was because property is a method, with variables, works perfect!

@igormcoelho
Copy link
Contributor Author

Are tests still broken? I'll take a look.

@shargon
Copy link
Member

shargon commented Aug 29, 2019

Formatted done!

@vncoelho
Copy link
Member

Still something:

 Error Message:
   CollectionAssert.AreEqual failed. (Different number of elements.)
  Stack Trace:
     at Neo.SmartContract.Framework.UnitTests.UT_OpcodeTest.TestAllOpcodes() in /home/travis/build/neo-project/neo-devpack-dotnet/tests/Neo.SmartContract.Framework.UnitTests/UT_OpcodeTest.cs:line 17

@shargon
Copy link
Member

shargon commented Aug 29, 2019

#99 solve the issue, but i think that UTF8 was not cleaned in this project

* Update neo opcodes

* Fix

* dotnet format

* Update OpCode.cs

* Remove SHA
@shargon
Copy link
Member

shargon commented Aug 29, 2019

Is fixed (i think) Thanks @vncoelho for the format

@erikzhang
Copy link
Member

We used to consider that struct is also a class when programming in C++. How is it in C#?

They are quite different in C#.

@lock9 lock9 merged commit 6756c40 into master Sep 1, 2019
@lock9 lock9 deleted the igormcoelho-invocation-counter branch September 1, 2019 10:58
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.

6 participants