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

add chain specific getMintInflation func #534

Merged
merged 2 commits into from
Jan 31, 2024
Merged

Conversation

ivivanov
Copy link
Contributor

Workaround for Customizing getMintInflation and Extending CosmosRestClient


Context

While integrating our application blockchain, I encountered two limitation:

  1. Inability to Override getMintInflation Function:
    The current design does not provide a straightforward method to override the getMintInflation function in a manner similar to customizing the request list. This limitation restricts our ability to inject custom logic or behaviors into the getMintInflation process.

  2. Extending CosmosRestClient:
    The existing architecture and design patterns do not readily accommodate the extension or modification of CosmosRestClient functionalities without substantial alterations to the core class.

Implemented Solution

To address these challenges, I have implemented a non-generic workaround. The core of this workaround involves a decision-making process based on the chain_name. This process determines which getMintInflation function to call, effectively allowing a form of method overriding based on the chain's name.

The implemented solution involves:

  • Introducing a conditional check within the relevant method or class.
  • Depending on the chain_name, the appropriate getMintInflation function is invoked – either the default implementation or a custom-defined function.

Request for Guidance

I am aware that this solution may not align with the standards and conventions your existing codebase.

I would greatly appreciate guidance or suggestions on how to implement this functionality more elegantly, in a manner that aligns with your codebase's architectural standards and design principles. My goal is to refine this implementation to be more generic, maintainable, and in harmony with our existing code structure.

Contribution and Collaboration

I am more than willing to iterate on this solution and work closely with the team to develop a more robust and standard-compliant approach. Any feedback, suggestions, or insights from the team would be invaluable in steering this implementation towards a more optimal and standardized solution.

@liangping
Copy link
Member

Thanks for contribution.
Please refer to this implementation.

mint_inflation: { url: '/evmos/inflation/v1/inflation_rate', adapter: (data: any) => ({inflation: (Number(data.inflation_rate || 0)/ 100 ).toFixed(2)}) },

mint_inflation: {

This would be the way we are expected.

Be free resubmit once you have implementation like this.

@ivivanov
Copy link
Contributor Author

ivivanov commented Jan 29, 2024

Thanks for the reply.

The lines which you are referring to is exactly the place where I started from. The issue I am facing with this approach is the lack of access to the instance of the CosmosRestClient where I can request the getStakingPool data or use the async request<T> function from the base class. Here is the way we calculate the staking APR. Additionally the adapter callback function is not async making it hard to make any get requests from it.

The bottomline is that I have to make 2 request in order to calculate the inflation

@liangping
Copy link
Member

You are trying to combinate two requests into one. you can change to adapter to async if you really want.

but, Here is inflation, not apr. would suggest to calculate APR somewhere else.

@ivivanov
Copy link
Contributor Author

Followed your advice and reworked the code to use async adapter. I moved the fetching and calculation logic inside the chain specific implementation.

It is inflation indeed. My bad...

Please let me know if I can improve my solution to fit the code standards of the repository.

@liangping liangping merged commit 4c53591 into ping-pub:master Jan 31, 2024
1 check passed
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.

2 participants