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

NeoToken: accept candidate registration via onNEP17Payment #3597

Merged
merged 1 commit into from
Dec 20, 2024
Merged

NeoToken: accept candidate registration via onNEP17Payment #3597

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 29 additions & 1 deletion src/Neo/SmartContract/Native/NeoToken.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,14 +332,42 @@ public BigInteger UnclaimedGas(DataCache snapshot, UInt160 account, uint end)
return CalculateBonus(snapshot, state, end);
}

[ContractMethod(Hardfork.HF_Echidna, RequiredCallFlags = CallFlags.States | CallFlags.AllowNotify)]
private async ContractTask OnNEP17Payment(ApplicationEngine engine, UInt160 from, BigInteger amount, StackItem data)
{
if (engine.CallingScriptHash != GAS.Hash)
throw new InvalidOperationException("only GAS is accepted");

if ((long)amount != GetRegisterPrice(engine.SnapshotCache))
throw new ArgumentException("incorrect GAS amount for registration");

var pubkey = ECPoint.DecodePoint(data.GetSpan(), ECCurve.Secp256r1);

if (!RegisterInternal(engine, pubkey))
throw new InvalidOperationException("failed to register candidate");
Copy link
Member

Choose a reason for hiding this comment

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

We don't burn it before throw?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

throw is like ABORT here, transaction will end up in FAULT state, nothing happened.

Copy link
Contributor

Choose a reason for hiding this comment

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

a reason why a want to update these exception name to uncatchableexception~~~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uncatchable can also be misunderstood, I can catch it in various places, it's just that VM/contracts are not even aware of this happening.

Copy link
Member

Choose a reason for hiding this comment

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

uncatachable should be named as normal. Name should be VMException with inner exception of normal name. Thats kind of how it done currently in the vm.


await GAS.Burn(engine, Hash, amount);
}

[ContractMethod(true, Hardfork.HF_Echidna, RequiredCallFlags = CallFlags.States)]
[ContractMethod(Hardfork.HF_Echidna, /* */ RequiredCallFlags = CallFlags.States | CallFlags.AllowNotify)]
private bool RegisterCandidate(ApplicationEngine engine, ECPoint pubkey)
{
if (!engine.CheckWitnessInternal(Contract.CreateSignatureRedeemScript(pubkey).ToScriptHash()))
// This check can be removed post-Echidna if compatible,
// RegisterInternal does this anyway.
var index = engine.PersistingBlock?.Index ?? Ledger.CurrentIndex(engine.SnapshotCache);
if (!engine.ProtocolSettings.IsHardforkEnabled(Hardfork.HF_Echidna, index) &&
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should allow it also with Echidna, it doesn't hurt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will require us keeping it forever then (in which case it's easier to duplicate the check). But my intention was to drop this check here eventually (keeping it in RegisterInternal only). But I'm OK with any option.

Copy link
Member

Choose a reason for hiding this comment

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

to drop this check here eventually

Vote up for this, we may remove this code after transition to Echidna, and the code will be more pretty. But we need an issue for that in order not to forget.

!engine.CheckWitnessInternal(Contract.CreateSignatureRedeemScript(pubkey).ToScriptHash()))
return false;
// In the unit of datoshi, 1 datoshi = 1e-8 GAS
engine.AddFee(GetRegisterPrice(engine.SnapshotCache));
Copy link
Member

@shargon shargon Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

why not return false if AddFee is not possible? Something like TryAddFee?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This won't help. The appropriate amount of GAS is already in the system fee (which is burnt forever before transaction is executed). If not, it'll fail and still burn whatever GAS is in the system fee.

Copy link
Member

Choose a reason for hiding this comment

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

This won't help. The appropriate amount of GAS is already in the system fee (which is burnt forever before transaction is executed). If not, it'll fail and still burn whatever GAS is in the system fee.

And create a new syscall, TryRegisterCandidate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How can it help? We need GAS. It either comes from the system fee or via a transfer. The first one burns the fee irrespective of the outcome. The second is what we have here.

The only other option is GAS.Transfer(pub_key_owner, ...) or even direct GAS.Burn(pub_key_owner, ...) call from NEO contract itself, but that's not the NEP-27 way. Transfers should be transfers and NEO deducting something from our GAS balance doesn't look nice.

Copy link
Member

Choose a reason for hiding this comment

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

Por testing you can try and for execute you can not try 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can find a global solution for it, maybe we have this problem in a different contract

And we have one. It's NEP-27. Allows to send any amounts of any token to some contract safely.

We can add extra gas for invokes that can only be burned, not consumed by opcodes

Once upon a time there was #2444. And then there was #2560. It can't work, system fee must be burned into ashes prior to execution.

Copy link
Member

Choose a reason for hiding this comment

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

Once upon a time there was #2444. And then there was #2560. It can't work, system fee must be burned into ashes prior to execution.

But this problem was different, this problem is testing the transaction with 1K GAS, but with a limited ExecutionEngine (1GAS for example)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This problem is #3552 (MaxBlockSystemFee) first of all. Solving RPC estimations is just a (very) nice side-effect.

Copy link
Member

Choose a reason for hiding this comment

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

The behaviour is different, before if was wrong signed, the fee was not burned

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The system fee is already gone by the time this executes (whether it's sufficient or not). The only thing left here is #3597 (comment), I can fix it, OK.

return RegisterInternal(engine, pubkey);
}

private bool RegisterInternal(ApplicationEngine engine, ECPoint pubkey)
{
if (!engine.CheckWitnessInternal(Contract.CreateSignatureRedeemScript(pubkey).ToScriptHash()))
return false;
StorageKey key = CreateStorageKey(Prefix_Candidate).Add(pubkey);
StorageItem item = engine.SnapshotCache.GetAndChange(key, () => new StorageItem(new CandidateState()));
CandidateState state = item.GetInteroperable<CandidateState>();
Expand Down
Loading
Loading