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

Create a hook to calculate the tx fee using given parameters #4712

Merged

Conversation

reyraa
Copy link
Contributor

@reyraa reyraa commented Jan 11, 2023

What was the problem?

Partially resolves #4716

How was it solved?

  • Create a utility function to calculate fee for a given transaction, account and priority options.
  • Create a utility to extract corresponding schema.
  • Create a useTransactionFee hook to calculate fees and return total + components

What is not included

  • Retrieve and include command fee in useTransactionFee
  • Enhance the hook to calculate values for
    • Minimum fee
    • Fee corresponding the selected priority
    • Fee corresponding maximum transferrable amount
  • Create a hook to identify the status of recipient account (initialized?)
  • Use above hook to calculate initialization fee and include it in the response (incorporate in total fee and add it to fee components)

How was it tested?

Unit tests

@reyraa reyraa self-assigned this Jan 11, 2023
@reyraa reyraa force-pushed the 4632-adapt-fee-computation-for-all-transactions branch from 7495094 to 94fdbe2 Compare January 16, 2023 11:47
@reyraa reyraa marked this pull request as ready for review January 16, 2023 11:47
@reyraa reyraa requested a review from soroushm January 16, 2023 11:48
@oskarleonard oskarleonard self-requested a review January 17, 2023 08:42
oskarleonard
oskarleonard previously approved these changes Jan 17, 2023
@reyraa reyraa force-pushed the 4632-adapt-fee-computation-for-all-transactions branch from bcb8763 to e66ffb1 Compare January 23, 2023 07:48
@ManuGowda ManuGowda self-requested a review January 23, 2023 09:06
Copy link
Contributor

@ManuGowda ManuGowda left a comment

Choose a reason for hiding this comment

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

one minor comment, please consider 🙏🏼

src/modules/transaction/hooks/useTransactionFee/utils.js Outdated Show resolved Hide resolved
soroushm
soroushm previously approved these changes Jan 23, 2023
ManuGowda
ManuGowda previously approved these changes Jan 23, 2023
@reyraa reyraa force-pushed the 4632-adapt-fee-computation-for-all-transactions branch from 3de4199 to 1df919e Compare January 23, 2023 13:35
isLoading: isSchemaLoading,
isFetched: isSchemaFetched,
} = useCommandSchema();
const schema = getParamsSchema(transaction, moduleCommandSchemas);
Copy link
Contributor

@oskarleonard oskarleonard Jan 23, 2023

Choose a reason for hiding this comment

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

Would be nice if we could use the same variable name in using functions/components. Makes easier when reading the code and following it to the function args of computeTransactionMinFee
const paramsSchema = getParamsSchema(transaction, moduleCommandSchemas);

data: auth,
isLoading,
isFetched,
} = useAuth({ config: { params: { address: senderAddress } } });
Copy link
Contributor

@oskarleonard oskarleonard Jan 23, 2023

Choose a reason for hiding this comment

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

Maybe we should add an enabled option for this hook and useCommandSchema, which checks for isValid ?

isLoading: isSchemaLoading || /* istanbul ignore next */ isLoading,
isFetched: isSchemaFetched && /* istanbul ignore next */ isFetched,
total: fee,
components: [{ value: fee, type: 'bytesFee' }],
Copy link
Contributor

Choose a reason for hiding this comment

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

How will this work?

const {components} = useTransactionFee(...);

return (<div>{components.map()}...

What i am wondering, do we really want this hook tightly coupled with components, feels like we are over abstracting it somehow 🤔

@reyraa reyraa merged commit d538a5d into feature/dpos-update Jan 23, 2023
@reyraa reyraa deleted the 4632-adapt-fee-computation-for-all-transactions branch January 23, 2023 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

4 participants