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

Fixes Deep Dive Docs Bounty Issue 598 #894

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

Conversation

rahul-soshte
Copy link
Contributor

Added a new category called Network Insights, in the Developer Tools, and added the link to the fee simulator tool, along with a nice description
Resolves issue #598

@rahul-soshte rahul-soshte changed the title Fixes Deep Dive Docs Bounty Issue #598 Fixes Deep Dive Docs Bounty Issue 598 Aug 7, 2024
@briwylde08
Copy link
Contributor

Awesome work! We'll get this assigned and reviewed 🎉

@sisuresh
Copy link
Contributor

sisuresh commented Aug 8, 2024

Hey @rahul-soshte, thanks for contributing. Some feedback and a request - Under estimation with Write Ledger Entries set to 5, it returns 5 xlm. It looks like the values make more sense once they're all filled in. The estimation tab is also missing parameters for rent.

Can you please demonstrate the calculations are right by making sure the result from Simulation matches up with what Estimation returns? I found a transaction on testnet, put it into simulation, and then put those values into Estimation, which returned a lower value (I'm guessing because the rent calculation was missing). Another thing to look into - the writeBytes shown on stellar.expert for the transaction I linked when you expand the arrows on the right is 3x higher than what your Simulation returns. I'm not sure why they're so different.

@rahul-soshte
Copy link
Contributor Author

Thank you @sisuresh for the feedback, I will dig more deep with the calculations and check what's missing in the overall calculations.

@sisuresh
Copy link
Contributor

sisuresh commented Aug 8, 2024

I'd also recommend taking a look at https://github.com/stellar/rs-soroban-env/blob/main/soroban-env-host/src/fees.rs.

@rahul-soshte
Copy link
Contributor Author

rahul-soshte commented Sep 3, 2024

Hi @sisuresh,

So I have researched the link that you gave, read the fee docs again, making sure what I am interpreting is correct. I have even open sourced the GUI repo ( used the laboratory frontend code as a base ), so it is much more faster to get feedback. Here are the changes/clarifications since the last time we talked,

  1. I am sending the writeBytes() from what I get from the the simulation endpoint properly here ( used the XDR for the testnet transaction that you sent ), might have been some bug in the stellar.expert

  2. What I am showing in the simulate fees route is the max resource fee. Do I need to add the actual estimate here as well?

  3. For the testnet tx that you sent, I am getting the fees bit close to the actual fee charged in the estimation. The actual fee charged was
    0.0357842 but I am getting around, 0.0319042, so I am close but don't know what mistake I am making

  4. In the rent calculation form, I have provided for a setting of the current ledger, and also you can add a ledger entry based on the following parameters of a ledger entry,
    a. Persistent or Temporary
    b. Old Size and New Size
    c. Old Live Until Ledger, New Live Until Ledger.
    I am sending this data in the fee simulation as well.

  5. Please do tell me what more mistakes I might be making as there are some nuances still involved here, which I am missing, or I might be making some small mistakes, which get the estimation slightly off.
    Rent Estimation GUI Code
    Other Resource Parameters Estimation GUI Code
    Simulation GUI code

@briwylde08 I am working on this tirelessly, it's just that there are lot many nuances than I thought. I am committed to completing this, and later extending it's scope so that it can be a good SCF project and also for maintenance of the GUI app, ad infinitum, since as Stellar grows, this GUI will need tweaks as well, based on the protocol changes.

@briwylde08
Copy link
Contributor

Hi @rahul-soshte! I completely respect that this is not an easy task and appreciate your continued work on it - a tool like this is super valuable to the ecosystem. I have no issues with this qualifying for the bounty (probably worth more than originally posted) and I agree that it would also make an awesome SCF project. I tagged @sisuresh for review again and the team will be in touch with feedback! Thanks again for your work!

@sisuresh
Copy link
Contributor

sisuresh commented Sep 5, 2024

Hey @rahul-soshte, thanks for the improvements. I put the testnet tx into Simulation, which returned the following -

"Fees":
{
"Max Resource fee (XLM)":
0.0372393,
"Max Estimated Fee (XLM)":
"0.0372493",
},

but Estimation returned 0.0293787 XLM. Do you know why there's a difference? Is that expected? I'm using 1398989 as the current ledger for both.

@rahul-soshte
Copy link
Contributor Author

rahul-soshte commented Sep 18, 2024

Hey @sisuresh Sorry for the delay got caught up in travelling somewhere and fell sick a bit.

So yeah I mean initially in the simulation, I was showing the max fee that simulation Endpoint returned instead of the actual estimated fee, which is wrong and confusing UX. So I now have fixed it. The simulation's estimated fee should match the direct estimation considering all the resource + rent value in the UI.

If you can find more bugs please do let me know. The Testnet tx we were using, has been wiped up by the network reset.

I am still working on fixing any bugs, and gonna research on how simulationTransaction endpoint is calculating the max resource fee. It so much higher than the actual fee charged. I am not sure why.
For example check this transaction
https://stellar.expert/explorer/testnet/tx/9947144265728
Here the actual fee charged is 0.005939
and the max fee is 0.0106407 which is way higher.

@rahul-soshte
Copy link
Contributor Author

rahul-soshte commented Sep 20, 2024

@sisuresh
Copy link
Contributor

sisuresh commented Oct 4, 2024

https://stellar.expert/explorer/testnet/tx/9947144265728

Hey @rahul-soshte, sorry for the delayed review. I wouldn't be surprised if users (or maybe even simulation) just adds a buffer for max fee to make sure their transaction will make it into the ledger, so I wouldn't worry about that (still worth looking into though as I bet it'll be educational!). I put a transaction from testnet into your Simulation tab and then into Estimation. The Estimate Total Fee matched between the two, so I think we're good there. The Analytics tab seems broken though. When I'm on the testnet option I see this under Analytics -

Fee Analytics (Mainnet)
Failed to fetch fee data for last 30 ledgers.

@rahul-soshte
Copy link
Contributor Author

rahul-soshte commented Oct 18, 2024

Hello @sisuresh,

Sorry for the late reply. I am trying to use this indexer project https://www.mercurydata.app/
It was working earlier with an example indexing deployed on zephyr, https://github.com/rahul-soshte/soroban-rough-work/blob/master/zephyr-fee-agg-imp/src/lib.rs

Zephyr is a great solution, but its like really new and things are breaking sometimes. Now I am getting error while fetching, earlier I was not getting errors while pulling data from a serverless function, which is written here in the deployed program

I am trying to pull the data from the serverless function here, in the simulator's frontend

I have commented out the analytics part. I wanted to do more work on the analytics part, since it was a interesting topic. I had also applied for a SCF#30 Build Award for this, but got rejected on that. Now got to form my own budget, and wait for the Zephyr ecosystem to mature more for the analytics part.

@briwylde08
Copy link
Contributor

Hi @rahul-soshte! Would just like to let you know that @sisuresh is out of office currently but will respond when he gets back!

@rahul-soshte
Copy link
Contributor Author

Okay @briwylde08 thanks for letting me know.

@sisuresh
Copy link
Contributor

sisuresh commented Nov 4, 2024

Hi @rahul-soshte, looks like this is in a good initial state to me. I'll pass the review onto @briwylde08 now. Thanks!

@briwylde08
Copy link
Contributor

Thanks @sisuresh for the review and @rahul-soshte on the work! I'm going have one of our devs on DevRel give this a final pass before approval.

@briwylde08
Copy link
Contributor

Hi @rahul-soshte! Just sent you a quick email using the address you provided in previous submissions. 🙏

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