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

Remove fixed fee #1125

Merged
merged 3 commits into from
Dec 5, 2023
Merged

Remove fixed fee #1125

merged 3 commits into from
Dec 5, 2023

Conversation

ureeves
Copy link
Member

@ureeves ureeves commented Nov 8, 2023

We substitute the spend function in the transfer contract for a spend_and_execute function, that also executes the contract call.

This fixes a bug where it was impossible to inform a genesis contract about whether they're being called in "management mode" by the VM - leading to sensitive functions being reachable by any contract call.

It also removes the fixed fee per transfer, since it was extremely artificial, and the regular transfer fee, while variable, is not much higher regardless.

@ureeves ureeves marked this pull request as ready for review November 8, 2023 19:32
@ureeves ureeves added fix:bug Something isn't working team:Core module:rusk Issues related to rusk module labels Nov 8, 2023
@herr-seppia herr-seppia self-assigned this Nov 8, 2023
HDauven
HDauven previously approved these changes Nov 8, 2023
Copy link
Member

@HDauven HDauven left a comment

Choose a reason for hiding this comment

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

LGTM, nice cleanup!

moCello
moCello previously approved these changes Nov 9, 2023
Copy link
Member

@moCello moCello left a comment

Choose a reason for hiding this comment

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

LGTM, only a few nits in the comments

contracts/transfer/src/state.rs Outdated Show resolved Hide resolved
contracts/transfer/src/state.rs Outdated Show resolved Hide resolved
rusk/src/lib/lib.rs Outdated Show resolved Hide resolved
rusk/src/lib/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@herr-seppia herr-seppia left a comment

Choose a reason for hiding this comment

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

As per discussion, tests should be added to verify consistency between receipt.gas_spent and wallet.balance

Apart from that, I've currently RFC due to a regression found with the following manual test:

  1. Run a local node
  2. Send a normal transfer with a very low gas (10k point are enough)
  3. Node is not able to create blocks due to hash mismatch between EST and VST

Tried the same with master, and transaction is properly discarded by EST and removed from the mempool

See also #1134

HDauven
HDauven previously approved these changes Nov 10, 2023
Copy link
Member

@HDauven HDauven left a comment

Choose a reason for hiding this comment

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

LGTM, that test is a nice addition. :)

rusk/tests/services/unspendable.rs Outdated Show resolved Hide resolved
Eduardo Leegwater Simões added 3 commits December 5, 2023 16:21
@ureeves
Copy link
Member Author

ureeves commented Dec 5, 2023

As per discussion, tests should be added to verify consistency between receipt.gas_spent and wallet.balance

Apart from that, I've currently RFC due to a regression found with the following manual test:

1. Run a local node

2. Send a normal transfer with a very low gas (10k point are enough)

3. Node is not able to create blocks due to hash mismatch between EST and VST

Tried the same with master, and transaction is properly discarded by EST and removed from the mempool

See also #1134

This has been addressed, and the fix is present in the latest master together with the a test that ensures it will never happen again.

Copy link
Member

@herr-seppia herr-seppia left a comment

Choose a reason for hiding this comment

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

LGTM

But I still believe that having a fixed fee for transfer would be a benefit for the network. Does it worth to reintroduce it?

@ureeves ureeves merged commit 053a492 into master Dec 5, 2023
3 checks passed
@ureeves ureeves deleted the rm-fixed-fee branch December 5, 2023 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix:bug Something isn't working module:rusk Issues related to rusk module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants