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

HIP-991 update according to latest discussion #1079

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

Conversation

netopyr
Copy link
Contributor

@netopyr netopyr commented Nov 14, 2024

Description:

This PR modifies the HIP according to the latest discussion:

  • Removed allowance (This will be handled in a later HIP.)
  • Added max_custom_fee instead

@netopyr netopyr self-assigned this Nov 14, 2024
@netopyr netopyr requested a review from mgarbs as a code owner November 14, 2024 11:36
Copy link

netlify bot commented Nov 14, 2024

Deploy Preview for hedera-hips ready!

Name Link
🔨 Latest commit fb13c6c
🔍 Latest deploy log https://app.netlify.com/sites/hedera-hips/deploys/673b04a2859f4800080a5323
😎 Deploy Preview https://deploy-preview-1079--hedera-hips.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

HIP/hip-991.md Outdated

### User Flows and Interaction

* Users will specify the fee settings during the topic creation process through a simple interface in their Hedera client (refer to the creation of token custom fees/fixed fee for reference).
* Before submitting a message to a topic through an application or wallet interface, users must set an allowance and a maximum fee per message.
* When submitting a message to a topic with custom fees through an application or wallet interface, users must set the maximum fee for the message.
Copy link
Contributor

Choose a reason for hiding this comment

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

to accommodate the requirement of accepting all fees, should we add a line that is something like:

  • Users submitting messages can add a flag to accept all custom fees from a topic id

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

@@ -232,6 +215,20 @@ message ConsensusCustomFeeList {
}
```

#### ConsensusSubmitMessageTransactionBody

The `ConsensusSubmitMessageTransactionBody` message is updated to include the optional `max_custom_fees` property for specifying the maximum fee that the user is willing to pay for the message.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to call out that there's a method to set accepts all fees in this section?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the flag accept_all_fees

Choose a reason for hiding this comment

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

It seems that the user must set either a max_custom_fees or set the accept_all_custom_fees to true. We can add a validation to throw an error if accept_all_custom_fees=false and max_custom_fees is not set.

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 think this is needed. If a topic has custom fees and neither max_custom_fees nor accept_all_custom_fees are set, the message will fail anyway because the fees cannot be paid.

Signed-off-by: Michael Heinrichs <[email protected]>
@netopyr netopyr requested review from ty-swirldslabs, Neurone, jasperpotts and rbair23 and removed request for ty-swirldslabs November 15, 2024 11:54
Signed-off-by: Michael Heinrichs <[email protected]>
HIP/hip-991.md Outdated
@@ -123,7 +108,7 @@ We propose adding a fixed fee mechanism to the Hedera Consensus Service (HCS) fo

##### Handling Duplicates

If the FEKL list contains duplicate keys, the transaction will fail with an `FEKL_CONTAINS_DUPLICATED_KEYS` error. This ensures that duplicate entries are not silently ignored, preventing potential bugs or issues in the calling code.
If the FEKL list contains duplicate keys, the `ConsensusUpdateTopicTransaction` transaction will fail with an `FEKL_CONTAINS_DUPLICATED_KEYS` error. This ensures that duplicate entries are not silently ignored, preventing potential bugs or issues in the calling code.

Choose a reason for hiding this comment

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

How about the duplicates in the ConsensusCreateTopicTransaction? This validation should apply to both create and update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, thanks. I will update the sentence.

HIP/hip-991.md Outdated

### User Flows and Interaction

* Users will specify the fee settings during the topic creation process through a simple interface in their Hedera client (refer to the creation of token custom fees/fixed fee for reference).
* Before submitting a message to a topic through an application or wallet interface, users must set an allowance and a maximum fee per message.
* When submitting a message to a topic with custom fees through an application or wallet interface, users must set the maximum fee for the message.

Choose a reason for hiding this comment

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

"users must set the maximum fee for the message" - when using the word must it seems that the field is required but the max_custom_fees is optional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the topic has custom fees and accept_all_custom_fees is false, a user must set max_custom_fees. In all other cases, the field is ignored and is therefore optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a sentence for clarification.

@vtronkov
Copy link

There are a few more mentions of "allowances" in the Mirror Node and SDK Reference Implementation sections. You can remove those.

Signed-off-by: Michael Heinrichs <[email protected]>
@@ -123,7 +108,7 @@ We propose adding a fixed fee mechanism to the Hedera Consensus Service (HCS) fo

##### Handling Duplicates

If the FEKL list contains duplicate keys, the transaction will fail with an `FEKL_CONTAINS_DUPLICATED_KEYS` error. This ensures that duplicate entries are not silently ignored, preventing potential bugs or issues in the calling code.
If the FEKL list contains duplicate keys, the `ConsensusCreateTopicTransaction`, respectively `ConsensusUpdateTopicTransaction`, will fail with a `FEKL_CONTAINS_DUPLICATED_KEYS` error. This ensures that duplicate entries are not silently ignored, preventing potential bugs or issues in the calling code.

Choose a reason for hiding this comment

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

The FEKL_CONTAINS_DUPLICATED_KEYS can be renamed to FEE_EXEMPT_KEY_LIST_CONTAINS_DUPLICATED_KEYS.

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