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

Feature: Dispenser Pallet #226

Merged
merged 14 commits into from
Apr 10, 2024
Merged

Feature: Dispenser Pallet #226

merged 14 commits into from
Apr 10, 2024

Conversation

vstam1
Copy link
Collaborator

@vstam1 vstam1 commented Apr 9, 2024

What?

This PR introduces the dispenser-pallet, that can be used to get locked PLMC tokens for account with a valid credential.

Why?

To enable initial user growth on the Polimec network, we allow a certain amount of users to dispense tokens.

How?

The pallet has a pallet account that can be funded to distribute tokens. The token amount that can be dispensed is specified in the DispenseAmount storage item and can be set by an admin account using the set_dispense_amount extrinsic. The initial amount is determined by the InitialDispenseAmount.

A user can call the dispense extrinsic to dispense tokens. There are a couple of checks to see if the user is able to dispense.
First, we check if the user has a valid credential and then we check if the Did in the credential has already dispensed tokens. We also check if the funding wallet still has tokens to distribute.

Signed Extensions for free execution

How do Signed Extensions work?

Every extrinsic that is submitted to a node, is first validated before it is added to the transaction pool and broadcasted to other nodes. The validation logic is written in Signed Extensions. If an extrinsic is valid, is being broadcasted to all other nodes. The validity of extrinsics is being tracked while it's in the transaction pool (it can get invalid by passing its mortality time for example). Once a collator starts building a block with this extrinsic, it first executes the pre_dispatch check, which should have similar logic as the validation check.

How to use SignedExtensions for free execution (in theory)?

Parity introduced the new SkipCheckIfFeeless signed extrinsic which is a wrapper around other signed extensions. A call can be marked as potentially free using feeless_if (see dispense extrinsic). The feeless_if implementation has some logic attached to see if the call is actually free. If feeless_if returns true, SkipCheckIfFeeless skips the inner signed extension check. This can be used to skip the ChargeTransactionPayment extension, which is responsible for charging fees. So in Theory, just wrapping this ChargeTransactionPayment extension with SkipCheckIfFeeless and correctly implementing feeless_if, should make the call free.

Free execution in practice?

SkipCheckIfFeeless doesn't work out of the box. There were two problems we ran into:

  • SkipCheckIfFeeless did not implement the validate function, which would skip the validation logic for fee payment for every transaction. So transactions were only checked for fee payment in the pre_dispatch function, which opens up a transaction pool DoS attack vector. We fixed this by implementing this Signed Extension ourselves with the validate function. Parity also implements it now: Fixes validation for SkipCheckIfFeeless extension paritytech/polkadot-sdk#3993.

  • The CheckNonce extension has a check that the user paid for the Nonce storage by checking if the accounts providers and sufficients are non-zero. For new accounts these are zero, so we had to skip this check for the dispense call. So I implemented the CheckNonce signed extension myself, with this extra logic. Parity acknowledged this problem and is discussing a general solution at CheckNonce should not enforce the account creation requirement for feeless calls paritytech/polkadot-sdk#3991. I think we are fine with our current implementation, as users actually receive tokens that will be locked/vested over a period of 4 years and the accounts able to get the tokens are limited by the credentials.

Testing?

Normal unit tests + a test to check the Signed extensions in the integration tests.

Screenshots (optional)

Anything Else?

Todo:

  • Change the Admin account?

@vstam1 vstam1 marked this pull request as ready for review April 9, 2024 11:17
@vstam1 vstam1 changed the title Feature/dispenser pallet Feature: Dispenser Pallet Apr 9, 2024
Copy link
Contributor

@JuaniRios JuaniRios left a comment

Choose a reason for hiding this comment

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

Most important thing to test out is that the extrinsic can actually be called for free, and we are not really testing that in the pallet test because all extrinsics are free.

After the pallet test actually tests that, I suggest testing it by running a local node with zombienet.

pallets/dispenser/src/lib.rs Outdated Show resolved Hide resolved
pallets/dispenser/src/lib.rs Show resolved Hide resolved
pallets/dispenser/src/tests.rs Show resolved Hide resolved
mod dispense {
use super::*;
#[test]
fn user_can_dispense_for_free() {
Copy link
Contributor

Choose a reason for hiding this comment

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

The mock runtime doesn't charge a fee for executing extrinsics. I assume this is to make pallet tests easier since a lot of pallets modify your balance outside of the fees charged.
You can double-check this with this simple test:

	#[test]
	fn sandbox() {
		ExtBuilder::default().build().execute_with(|| {
			assert_eq!(Balances::free_balance(1), 0);
			assert_ok!(System::remark(RuntimeOrigin::signed(1), vec![69, 69]));
		});
	}

I think you need to modify the mock runtime to actually charge fees (if that is even possible)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why exactly? We are testing the correct execution of the logic within the extrinsics. For the "free execution" tests, we have an integration test that checks the validate and pre_dispatch execution. And I ran the extrinsic on zombienet to see if execution was free, and this worked. So I thing these three together should show the correct working of the pallet.

Copy link
Contributor

Choose a reason for hiding this comment

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

the test is called user_can_dispense_for_free but you are not testing that because there are no fees in this mock runtime, so if you think this is tested in other places like integration-tests, then I would remove this test and maybe leave a comment where this test can be found.

I see though that we have 2 dispenser tests called dispenser_signed_extensions_pass_for_new_account and dispenser_works_with_runtime_values But I don't see it testing that a user with 0 balance can call dispense, while other calls would require fees/ED.

Copy link
Member

@lrazovic lrazovic left a comment

Choose a reason for hiding this comment

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

Love to see the feeless_if + JWT check in action!

The logic looks good, just few comments here and there that I would like to discuss before merging this into main.

Also, the try-runtime-cli is complaining about the Funding Pallet: Funding: On chain storage version StorageVersion(1) is set to non zero, while the pallet is missing the #[pallet::storage_version(VERSION)] attribute. So maybe we should rebase this PR to include #223

pallets/dispenser/src/lib.rs Outdated Show resolved Hide resolved
pallets/dispenser/src/lib.rs Outdated Show resolved Hide resolved
pallets/dispenser/src/lib.rs Show resolved Hide resolved
pallets/dispenser/src/tests.rs Show resolved Hide resolved
runtimes/politest/src/lib.rs Show resolved Hide resolved
pallets/dispenser/src/lib.rs Outdated Show resolved Hide resolved
pallets/dispenser/src/lib.rs Show resolved Hide resolved
@JuaniRios JuaniRios merged commit 979ab00 into main Apr 10, 2024
@JuaniRios JuaniRios deleted the feature/dispenser-pallet branch April 10, 2024 12:01
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.

3 participants