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

Improve system contract loading reliability #264

Conversation

dallasjohnson
Copy link
Contributor

No description provided.

@dallasjohnson
Copy link
Contributor Author

@thekevinbrown Are you guys still maintaining this. Just wondering if I should just use my own fork if you have moved on to different projects now.

@thekevinbrown
Copy link
Member

@dallasjohnson, yes, we're still maintaining this, sorry for the delay. I'll have a look today.

Copy link
Member

@thekevinbrown thekevinbrown left a comment

Choose a reason for hiding this comment

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

More questions than anything.

Thanks again for your contribution, much appreciated!

},
});
}
// Same deal for delegatebw. Only if it's actually a thing.
if (systemContract.actions.has('delegatebw')) {
const delegateAmount = '10.0000 EOS';
Copy link
Member

Choose a reason for hiding this comment

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

From memory trying to use 10 EOS was breaking with our test chain setup, which is why we were using 10 SYS. Have the later versions of EOS changed this? (e.g. Have you tested that this definitely works? If so, that's all I need, just wanted to check because I remember it causing issues before.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This worked for me. I was ususally seeing issues where the system contract would not actually get set which is why had delays and retries to get it to work. It also fail silently when it didn't get set which made it even worse to bugfix.

Copy link
Member

Choose a reason for hiding this comment

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

Cool, as long as it's working for you I'm good. I think the SYS token is the default if you don't have some of the system contracts set up (which I think you now have set up), so the more we can be like mainnet the better in my opinion. 👍

cleos set contract eosio "$contracts_dir/eosio.system" -p eosio@active
sleep 0.5s
cleos set contract eosio "$contracts_dir/eosio.system" -p eosio@active
sleep 0.5s
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be set 3 times?

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'm ashamed of this 😞 but since setting the system contract would often fail without error I found setting it multiple times with a small delay made it work. I really don't like it and would prefer a different option but I was very surprised to see that all my EOSDAC tests had been running without the system contract getting set. It wasn't until I was working on a custom eosio.msig without inner deferred transactions that needed privileged access that I realised I needed this to be working. I would love a better suggestion because other than just being dirty code it also adds delay to start of tests.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this feels like something we can ask Block One about, but I've found similar flakiness with EOS setup tasks. Things that should be deterministic just often aren't, then when you ask for how they can automatically spin up a new mainnet with an identical setup they point you to an instruction sheet instead of a script or program. Quite frustrating, definitely not your fault. I'll add a comment noting the reason for this, otherwise it's in a fairly black box part of the system so it shouldn't get in too many people's way.

src/scripts/init_blockchain.sh Outdated Show resolved Hide resolved
Copy link
Member

@thekevinbrown thekevinbrown left a comment

Choose a reason for hiding this comment

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

Thanks again!

@thekevinbrown thekevinbrown merged commit 1a12cc0 into CoinageCrypto:master Aug 10, 2020
@thekevinbrown
Copy link
Member

Just wanted to let you know that this has been released (along with a bunch of dependency updates) in lamington@1.0.0-alpha.6.

@dallasjohnson dallasjohnson deleted the Improve-system-contract-loading-reliability branch August 10, 2020 12:29
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