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

Refuel #2444

Merged
merged 4 commits into from
Apr 24, 2021
Merged

Refuel #2444

merged 4 commits into from
Apr 24, 2021

Conversation

erikzhang
Copy link
Member

Close #2443

@erikzhang erikzhang added this to the NEO 3.0 milestone Apr 21, 2021
@shargon shargon mentioned this pull request Apr 21, 2021
Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

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

@superboyiii tested?

Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

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

I will add some UT

Copy link
Contributor

@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.

@erikzhang @shargon interesting implementation.
Could/should we launch some Notification here during Refuel? Imagine a contract is giving 0.1 GAS every time a user invokes "NEP17.transfer()". But suppose the contract is willing to give that a single time per execution... should contract itself launch a Notification (and observe it later somehow) or having this process automatically done here?

@erikzhang
Copy link
Member Author

But suppose the contract is willing to give that a single time per execution...

Use invocationCounter?

@roman-khimov
Copy link
Contributor

Also Burn is supposed to emit a transfer notification, of course it's not exactly fun to parse notifications, but still it's a possibility.

@igormcoelho
Copy link
Contributor

Use invocationCounter

That's true @erikzhang, so many ways to handle this 😂

{
sb.EmitDynamicCall(NativeContract.GAS.Hash, "refuel", accBalance.ScriptHash, 100 * NativeContract.GAS.Factor);
sb.Emit(OpCode.DROP);
sb.EmitSysCall(ApplicationEngine.System_Runtime_GasLeft);
Copy link
Contributor

Choose a reason for hiding this comment

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

How much Gas is expected to be here? Maybe a comment would help.

Scopes = WitnessScope.CalledByEntry
} };

var tx = wallet.MakeTransaction(snapshot, script, accBalance.ScriptHash, signers);
Copy link
Contributor

Choose a reason for hiding this comment

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

How much gas is added here automatically?

engine.LoadScript(tx.Script);
Assert.AreEqual(VMState.HALT, engine.Execute());
Assert.AreEqual(1, engine.ResultStack.Count);
Assert.AreEqual(100_00300140, engine.ResultStack.Pop().GetInteger());
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be precisely 100? I mean, if tx fee value is exact, and 100 is added up, we should perhaps rest with 100, right? Anyway, value is close to it.

Copy link
Contributor

@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.

Beautiful implementation, so concise and precise.

@erikzhang
Copy link
Member Author

Merge?

@nicolegys
Copy link
Contributor

I don't think it's user-friendly :(. User needs to guess how many gas he needs to add.

@ArithMegatron
Copy link

Does it has example to use in client side? I'm not sure how to get the exact gas-used while the scripts are not determined.

@igormcoelho
Copy link
Contributor

igormcoelho commented Apr 22, 2021

I don't think it's user-friendly :(. User needs to guess how many gas he needs to add.

@nicolegys that's not up to the user, but to the contract (and wallet) developer. The way I see it, if any existing wallet is able to estimate GAS costs (including variable storage, loops, etc) then it will be able to deduce the effect of Refuel operations (and subtract from final spent GAS). I think that these things are not trivial on general, as they approach the theoretical limits of the Halting Problem, so contract developers should provide guidelines on how much GAS to send to their own contracts, specially if they are willing to sponsor user operations (this may require special plugins for advanced NEP-17 on existing wallets or proprietary wallets/interfaces).

Let me give you some example that comes to my mind: imagine some NEP-17 is topping up gas for users, during "transfer" operation. Then it could properly inform this to users and allow that transactions cover only a minimum amount of initial GAS (the rest is introduced by the contract itself). How much GAS is required? I agree that this is something contract-specific and somehow the contract developer should pass it on to general wallet developers, unless these wallets allow users to manually control GAS (in that I agree with you, that becomes a user responsibility). But I think that providing a feature on Neo layer just gives the possibility for wallets/business to explore this feature. So this is a future discussion that should include wallet developers, specially for NEP-17 contracts and other standard tokens.

Does it has example to use in client side? I'm not sure how to get the exact gas-used while the scripts are not determined.

@ArithMegatron the best example I could think is to invoke Native NEO transfer without proper amount of GAS, and loading more GAS (from the user) on Entry script. Quite similar to the one added to unit tests.

@igormcoelho
Copy link
Contributor

@nicolegys and @ArithMegatron , I've thought a little bit more about it, and that's really an interesting issue. Maybe the best way to be very efficient of this matter is to provide wallet plugins for GAS calculation, that are contract-specific. This allows different groups that want to properly calculate their operations (and maybe give some free GAS), to insert their contract-specific codes inside general wallets.

erikzhang added a commit that referenced this pull request Aug 4, 2021
This reverts commit 4d45a79.
erikzhang added a commit that referenced this pull request Aug 5, 2021
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.

GAS reloading on execution
6 participants