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

Add nep11 tracker #651

Closed
wants to merge 31 commits into from
Closed

Conversation

Ashuaidehao
Copy link
Contributor

@Ashuaidehao Ashuaidehao commented Oct 9, 2021

Close #542
Add nep11 tracker.

Copy link
Contributor

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

It technically closes #542, so it should be mentioned somewhere.

src/RpcNep11Tracker/Storage/Nep11Balance.cs Show resolved Hide resolved
}

[RpcMethod]
public JObject GetNep11Transfers(JArray _params)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just FYI, take a look at NeoGo extensions for getnep17transfers (limits and paging): https://github.com/nspcc-dev/neo-go/blob/master/docs/rpc.md#limits-and-paging-for-getnep17transfers

We'll make them available for NEP-11 tracking also, but maybe C# node can make use of them too.

src/RpcNep11Tracker/RpcNep11Tracker.cs Outdated Show resolved Hide resolved
src/RpcNep11Tracker/RpcNep11Tracker.cs Outdated Show resolved Hide resolved
src/RpcNep11Tracker/RpcNep11Tracker.cs Outdated Show resolved Hide resolved
src/RpcNep11Tracker/RpcNep11Tracker.cs Show resolved Hide resolved

ResetBatch();

ushort transferIndex = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have it block-level instead of execution context (transaction, basically) level?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nep11TransferKey will use it to distinguish different transfers with same parameters.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. We've changed NEP-17 tracker storage scheme long time ago and it's no longer needed for this. This data at the same time just seems a little awkward, it's returned in transfernotifyindex and I have no idea what user can do with it. Maybe we can keep it internal then, not return in JSON? To me, more valuable thing to have here would be transaction-level index (so that we could getapplicationlog and see the original notification event for this transfer).

Copy link
Contributor

Choose a reason for hiding this comment

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

And BTW, if it's block-level now then ushort might be a bit too short, technically the protocol allows for 64K transactions in a block (and maybe even more after neo-project/neo#2300, 64K was a hard limit around neo-project/neo#1937) and every transaction can emit a number of events.

src/RpcNep11Tracker/RpcNep11Tracker.cs Outdated Show resolved Hide resolved
src/RpcNep11Tracker/RpcNep11Tracker.cs Outdated Show resolved Hide resolved
@superboyiii
Copy link
Member

superboyiii commented Oct 19, 2021

Test Pass
From my side, it solved current requirements of showing Nep11 tx history (sorted by timestamp) and current NFTs in specific address with group by contract hash. But still need @roman-khimov and @shargon review again.

superboyiii
superboyiii previously approved these changes Oct 19, 2021
@superboyiii superboyiii requested a review from shargon October 19, 2021 03:01
{
public readonly UInt160 UserScriptHash;
public readonly UInt160 AssetScriptHash;
public ByteString Token;
Copy link
Member

Choose a reason for hiding this comment

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

Is not better to use Nep17BalanceKey or join both in NepBalanceKey with extras, instead of having two plugins ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean merge nep17 and nep11 tracker?

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 it could me done easily, but what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will be a heavy plugin, @superboyiii what do you think?

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 no need to merge. Nep11 Tracker and Nep17 Tracker are different requirements. As I know many exchanges only support Nep17, no requirements for Nep11 as well as some wallets.

src/RpcNep11Tracker/Storage/Nep11Balance.cs Outdated Show resolved Hide resolved
src/RpcNep11Tracker/RpcNep11Tracker.cs Show resolved Hide resolved
src/RpcNep11Tracker/RpcNep11Tracker.cs Show resolved Hide resolved
src/RpcNep11Tracker/Storage/Nep11Transfer.cs Outdated Show resolved Hide resolved
@superboyiii
Copy link
Member

Test Pass:
neoSystem.LoadStore works well after fixed.

superboyiii
superboyiii previously approved these changes Oct 25, 2021
@superboyiii
Copy link
Member

superboyiii commented Oct 28, 2021

Test PASS
[SupportedStandards("NEP-11")], [SupportedStandards("nep-11")], [SupportedStandards("Nep-11")].... these could successfully be tracked. Others like [SupportedStandards("nep11")] will fail.

@roman-khimov
Copy link
Contributor

I'd actually only allow NEP-11, see neo-project/proposals#139, we should have exactly one proper way of writing this for contracts to follow. Otherwise we'll inevitably have a lot of variations among different contracts and everyone using this data will have to implement support for all of them.

@superboyiii
Copy link
Member

I'd actually only allow NEP-11, see neo-project/proposals#139, we should have exactly one proper way of writing this for contracts to follow. Otherwise we'll inevitably have a lot of variations among different contracts and everyone using this data will have to implement support for all of them.

neo-project/proposals#142

@vncoelho
Copy link
Member

@Ashuaidehao, the consensus and nodes stopped syncing when testing this PR.

@superboyiii
Copy link
Member

@Ashuaidehao, the consensus and nodes stopped syncing when testing this PR.

Could you tell us more details? I can't replay your issue.

@vncoelho
Copy link
Member

vncoelho commented Nov 1, 2021

@superboyiii,

testing the following config:

# https://github.com/neo-project/neo-node/commits/master
NEO_CLI_BLOCKCHAIN_URL=https://github.com/neo-project/neo-node.git
NEO_CLI_BRANCH=master
NEO_CLI_COMMIT=8153553dcd8d68c752b9988ce7d7b9e360529b65

# https://github.com/neo-project/neo/commits/master
NEO_BLOCKCHAIN_URL=https://github.com/Qiao-Jin/neo.git
NEO_BLOCKCHAIN_BRANCH=taskmanager_optimization
NEO_BLOCKCHAIN_COMMIT=456fe0242001eec2ce92b44fbba84c5d5e9393b1

# https://github.com/neo-project/neo-modules/commits/master
NEO_PLUGINS_URL=https://github.com/neo-project/neo-modules.git
NEO_PLUGINS_BRANCH=master
NEO_PLUGINS_COMMIT=d0f143ac43fe6d4496430c01f737f3dd9f232904

Worked.
Now, when changing to:

# https://github.com/neo-project/neo-modules/commits/master
NEO_PLUGINS_URL=https://github.com/Ashuaidehao/neo-modules.git
NEO_PLUGINS_BRANCH=add-nep11
NEO_PLUGINS_COMMIT=7556a43

It stopped synced and stuck on changeview.

In fact, it looks like it is because the *.dll that have dependency on RpcServer can not be on the folder if the node does not have RpcServer enabled.
Since two out of four of our CNs do not have RpcServer, I noticed that I had to also delete it before starting the node:

if [[ ${IS_RPC_SERVER} = "0" ]]; then
	echo "Deleting RPCServer related plugins"
	rm /opt/node/neo-cli/Plugins/RpcServer.dll
	rm /opt/node/neo-cli/Plugins/RpcNep17Tracker.dll
	rm /opt/node/neo-cli/Plugins/RpcNep11Tracker.dll
	rm /opt/node/neo-cli/Plugins/ApplicationLogs.dll
	rm /opt/node/neo-cli/Plugins/StateService.dll	
	sleep 1
fi

In this sense, the problem is more generic than this PR,it is a result of not having the RpcServer.
Perhaps the *.dll could be in the folder and we could verify if the config file is created or node, something like that.
Anyway, the problem was on our side here by not deleting the * .dll for some nodes.

@superboyiii
Copy link
Member

superboyiii commented Nov 3, 2021

In this sense, the problem is more generic than this PR,it is a result of not having the RpcServer. Perhaps the *.dll could be in the folder and we could verify if the config file is created or node, something like that. Anyway, the problem was on our side here by not deleting the * .dll for some nodes.

It's an issue about depedency check on neo-core. Since all RpcServer required plugins have this issue, we could make it into another PR to check dependency before onload. Currently, for this PR I think it's OK. What do you think?

superboyiii
superboyiii previously approved these changes Nov 3, 2021
Console.WriteLine($"Fault:{record.from},{record.to},{record.tokenId.GetSpan().ToHexString()}");
return;
}
Put(Nep11BalancePrefix, new Nep11BalanceKey(record.to, record.asset, record.tokenId), new Nep11Balance() { Balance = engine.ResultStack.Pop().GetInteger(), LastUpdatedBlock = _currentHeight });
Copy link
Member

Choose a reason for hiding this comment

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

Is not easy to deny the service if the contract is prepared to return a wrong result? it will deny the service to any node that will have this plugin, I think that it should log the error but not deny the service, imagine if a consensus node has this plugin.

https://github.com/neo-project/neo-vm/blob/b48113f8dc787d059324c2d34c855f77b0e3645d/src/neo-vm/Types/StackItem.cs#L116

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please check #671

@Ashuaidehao Ashuaidehao mentioned this pull request Nov 4, 2021
@superboyiii
Copy link
Member

I will close this since it's replaced by #671

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.

NEP-11 tracker
7 participants