Skip to content

feat(fees): Add fee receiver #5333

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

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

aoyako
Copy link
Contributor

@aoyako aoyako commented Feb 27, 2025

Adds fee receiver options. New functionality can be access by upgrading the executor to fees_executor (See example in tests).
Fees are read-only at this moment.

Default fees options are obtained from the environment variables and can be changed when building the executor.

Some missing features (will be added in this PR):

  • A logic to ensure that the fee receiver/asset/domain could not be unregistered (Or some disable mechanism should be implemented).

Signed-off-by: Lohachov Mykhailo <[email protected]>
@aoyako aoyako marked this pull request as draft February 27, 2025 06:51
Copy link
Contributor

@mversic mversic left a comment

Choose a reason for hiding this comment

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

Looks like a step in the right direction

pub asset: AssetDefinitionId,
}

impl Default for FeesOptions {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that we can have this default. You should find a different approach IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed so they are obtained from the environment variables.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it matter if the environment variable FEES_ASSET differs from peer to peer?

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 envis used only when building the executor and is the uploader's responsibility. It shouldn't be affected by individual peers' configurations.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well I'll be damned. This is not a bad solution at all

Signed-off-by: Lohachov Mykhailo <[email protected]>
Signed-off-by: Lohachov Mykhailo <[email protected]>
@aoyako aoyako self-assigned this Feb 28, 2025
@aoyako aoyako marked this pull request as ready for review February 28, 2025 05:46
Signed-off-by: Lohachov Mykhailo <[email protected]>
@s8sato s8sato linked an issue Mar 3, 2025 that may be closed by this pull request
Copy link
Contributor

@s8sato s8sato Mar 3, 2025

Choose a reason for hiding this comment

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

I don't understand why this custom executor and custom visit_* logic are necessary. What advantage do they have over default permissions? Could you clarify the requirements in #5315 @aoyako @mversic ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the question here. How would you do it otherwise? It's an executor that controls fees. Every tx will run through it and it will produce a fee deduction

Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned in #5315 (comment), I now support using an executor and am questioning the necessity of customizing permission validations.
Would it not be sufficient to simply withhold permissions for modifying specific parameters, domains, accounts, and asset definitions?

pub asset: AssetDefinitionId,
}

impl Default for FeesOptions {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it matter if the environment variable FEES_ASSET differs from peer to peer?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the question here. How would you do it otherwise? It's an executor that controls fees. Every tx will run through it and it will produce a fee deduction

@0x009922 0x009922 self-assigned this Mar 11, 2025
Copy link
Contributor

@0x009922 0x009922 left a comment

Choose a reason for hiding this comment

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

LGTM. If we are sure about the model when there is a single fee receiver in the network, this PR seems to do the job.

0x009922
0x009922 previously approved these changes Mar 17, 2025
@aoyako aoyako marked this pull request as draft March 25, 2025 11:58
@aoyako aoyako marked this pull request as ready for review March 27, 2025 06:00
@github-actions github-actions bot added the config-changes Changes in configuration and start up of the Iroha label Mar 27, 2025
Copy link

@BAStos525

aoyako added 2 commits March 27, 2025 22:22
Signed-off-by: Lohachov Mykhailo <[email protected]>
Signed-off-by: Lohachov Mykhailo <[email protected]>
Comment on lines +411 to +413
// client.submit_all_blocking(upgrade_executor_isi(executor))?;
for isi in upgrade_executor_isi(executor) {
client.submit_blocking(isi)?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know for now why in the submit_all case, the executor complains about a lack of fuel.

@s8sato s8sato self-assigned this Apr 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
config-changes Changes in configuration and start up of the Iroha
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add technical account that will receive network fees
4 participants