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

Adding Source Protocol to networks.json #754

Merged
merged 7 commits into from
Dec 1, 2023

Conversation

zenodeapp
Copy link
Contributor

@zenodeapp zenodeapp commented Nov 2, 2023

Hi!

Here's a request to add the Source protocol to the networks.json file. I made sure to only add the necessary values in order for autostaking (or transactions in general) to work; which are the gasPrice and gasModifier.

Have a great day!

PS: I didn't see any other network with the gasModifier value set, but I presume it should work since some have the autostake object included.


https://sourceprotocol.io
https://restake.app/source

@zenodeapp
Copy link
Contributor Author

zenodeapp commented Nov 2, 2023

UPDATE: Ah! Think I already got the answer, saw that lodash's merge and mergeWith methods are being used!


One question:
I saw that arrays don't get merged, but nothing about deeper objects like "autostake". So my question is whether we will be able to add "autostake" values in our local network files without it overwriting the gasModifier value? Unless it's explicitly overwritten of course. Asking this because I'm unsure how the final configuration gets merged together.

Is it a deep merge like this example with the use of spread operators?
{...sourceNetwork, ...sourceNetworkLocal, "autostake": {...sourceNetwork.autostake, ...sourceNetworkLocal.autostake}

Or will adding an "autostake" object in the networks.local.json file completely overwrite the one in the networks.json file?
{...sourceNetwork, ...sourceNetworkLocal}

@zenodeapp
Copy link
Contributor Author

zenodeapp commented Nov 5, 2023

Hey, so I think a few more tests have to be run and the gas issues have to be sorted out before actually merging this. Some manage to restake without actually having to fiddle with gas settings, but not certain if it's consistent.

Is it okay to keep this PR open? Or you prefer to have this PR closed and reopened at a later time (if this still is a necessity)?

@tombeynon
Copy link
Contributor

@zenodeapp thanks for this!

Chain Registry suggests the 'low' gas price is 0.05usource - does using that avoid the need to set the gasModifier? Main reason being it gets expensive quickly if the modifier is set too high, and I haven't had to adjust this for any other chains so far.

@zenodeapp
Copy link
Contributor Author

@zenodeapp thanks for this!

Chain Registry suggests the 'low' gas price is 0.05usource - does using that avoid the need to set the gasModifier? Main reason being it gets expensive quickly if the modifier is set too high, and I haven't had to adjust this for any other chains so far.

So sorry, just seeing this. Let me setup my validator to use such settings and I'll report back if it works fine or fails too often. (Even mine fails every once in 3-5 days, but are chain issues that probably need to be sorted out).

Bear with me! :)

@zenodeapp
Copy link
Contributor Author

zenodeapp commented Nov 21, 2023

I already changed the value to 0.05usource, without modifier. I have this configured locally at the moment and set my restake frequency to 1hr. If it fails my healthchecks will alert me!

Up till now manually auto-staking worked fine, but will need to test how often it fails. For those with a higher frequency rate it's no biggy if it fails every now and then, but for those who autostake less frequently it could become more of a problem. Perhaps upping the retries value would solve this, we tend to get the answer from the devs to retry when a transaction fails.

I'll keep ya posted :)!

@zenodeapp
Copy link
Contributor Author

zenodeapp commented Nov 22, 2023

Okay, 1 day passed and so far I've had one fail happen out of ~24 successful ones.

image

This is normal (well not really normal, but expected).

It seems to cost 0.06 SOURCE per autostake now though versus 0.04 SOURCE, https://ping.pub/source/tx/CAC617AE1915994F2F5447D48E107E906D13852720F9C890AD1CCB8E3391058B (transaction before my change to 0.05usource) vs now: https://ping.pub/source/tx/16645FEE5DA03932CBA26232D0165313BA8C9A3CFDB9585EFF7F19593954058B

This is no biggy though! If it's better to leave gasModifier untouched then I believe this setting should be okay. I may need to play around with a higher value for retries, perhaps this will prevent it from failing too often (especially for people who have a less frequent restake).

I'll report back in a couple days! Have a good day man.

@zenodeapp
Copy link
Contributor Author

zenodeapp commented Nov 23, 2023

@tombeynon

I also changed retries to 10. I saw in my logs that one of the autostake attempts was at its 7th attempt (of 11 attempts) before it actually succeeded!

So the combi of gasPrice on 0.05usource and a higher retries value might be a better fix to prevent it from failing. And if it fails even after 11 attempts then...well, that would be more so an issue with the network rather than a problem we should fix.

Let me know what you think. If you agree, then I'll add the retries value also to the networks.json file (probably very uncommon, so therefore want to ask you first before I add it into this PR).

If you'd like to see the logs, tell me. At the moment I can't share it as I'm not behind a pc and this phone is giving me a hard time copying the logs in the Termux app :').

@zenodeapp
Copy link
Contributor Author

zenodeapp commented Nov 24, 2023

@tombeynon

I also changed retries to 10. I saw in my logs that one of the autostake attempts was at its 7th attempt (of 11 attempts) before it actually succeeded!

So the combi of gasPrice on 0.05usource and a higher retries value might be a better fix to prevent it from failing. And if it fails even after 11 attempts then...well, that would be more so an issue with the network rather than a problem we should fix.

Let me know what you think. If you agree, then I'll add the retries value also to the networks.json file (probably very uncommon, so therefore want to ask you first before I add it into this PR).

If you'd like to see the logs, tell me. At the moment I can't share it as I'm not behind a pc and this phone is giving me a hard time copying the logs in the Termux app :').

Here's the log for reference: restake-source.log

@zenodeapp
Copy link
Contributor Author

zenodeapp commented Nov 29, 2023

Added retries 10 to the PR. Had no issues so far :)!

If the way I've configured it is not exactly how you had it in mind, tell me.

@tombeynon
Copy link
Contributor

@zenodeapp The retries are only really required here because you're using public nodes, which can be unreliable for two reasons.

  1. they get overloaded, which is why you see a few timeouts in your log.
  2. you can't control the node's gas price, which can vary and is why sometimes you see failures for gas price.

For this PR we should remove the retries option and leave that up to the user. I'll update the PR and merge so this is wrapped up, but raise an issue if you have any other problems.

@tombeynon tombeynon merged commit 12a522f into eco-stake:master Dec 1, 2023
@zenodeapp
Copy link
Contributor Author

Got it! Learning something new every time :)!

Thank you Tom.

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.

None yet

2 participants