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

Test staking #277

Merged
merged 20 commits into from
Mar 19, 2021
Merged

Test staking #277

merged 20 commits into from
Mar 19, 2021

Conversation

joelamouche
Copy link
Contributor

@joelamouche joelamouche commented Mar 10, 2021

What does it do?

  • add a config to test staking
  • add a script to test staking on 3 nodes

What important points reviewers should know?

run yarn run build-moonbeam-launch before running ts-node test-staking.ts in tools

Is there something left for follow-up PRs?

// TODO: leave_candidates
// TODO: ethan (added candidate) doesnt produce blocks => need to move blockPerRound to storage

What alternative implementations were considered?

Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?

What value does it bring to the blockchain users?

Checklist

  • Does it require a purge of the network?
  • You bumped the runtime version if there are breaking changes in the runtime ?
  • Does it require changes in documentation/tutorials ?

@joelamouche joelamouche marked this pull request as draft March 10, 2021 16:55
@joelamouche
Copy link
Contributor Author

joelamouche commented Mar 10, 2021

@4meta5 this is the branch im going to use to test staking

Copy link
Contributor

@4meta5 4meta5 left a comment

Choose a reason for hiding this comment

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

Nice, I want to work off this

@joelamouche
Copy link
Contributor Author

@4meta5 Added some tests.
Things that don't wok as expected:

// what is min amount? 100k is not enough, unlike amount specified in https://meta5.world/stake-docs/runtime.html
// TODO: ethan (added candidate) doesnt produce blocks
// TODO: nominator revokation doesnt work

@4meta5
Copy link
Contributor

4meta5 commented Mar 17, 2021

@joelamouche You cannot revoke a nomination if it leaves your remaining total nominations below MinNominatorStk -- the call to revoke_nomination does that so it returns an error. It gives the index of the error in the output:

Transaction finalized at blockHash 0x9333308d9a7e391bf35f0a774d99c9a754a40e50ac6359637ae34cadec1c3349
	' {"applyExtrinsic":3}: system.ExtrinsicFailed:: [{"module":{"index":11,"error":6}},{"weight":0,"class":"Normal","paysFee":"Yes"}]

So this is why there is no change after revoke_nomination. The call did not succeed so there were no storage changes.

In this case, leave_nominators must be called. This method has no inputs so it is easier to call.

@joelamouche
Copy link
Contributor Author

Thanks!
What do you think about those:

// what is min amount? 100k is not enough, unlike amount specified in https://meta5.world/stake-docs/runtime.html
// TODO: ethan (added candidate) doesnt produce blocks

@joelamouche joelamouche marked this pull request as ready for review March 17, 2021 16:20
@joelamouche
Copy link
Contributor Author

ready for review

@joelamouche joelamouche added A0-pleasereview Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low Does not elevate a release containing this beyond "low priority". labels Mar 17, 2021
Copy link
Contributor

@4meta5 4meta5 left a comment

Choose a reason for hiding this comment

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

Nice job!

Copy link
Contributor

@4meta5 4meta5 left a comment

Choose a reason for hiding this comment

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

Did you update test-staking-spec.json since the latest changes to staking were merged into master? I think the changes will make the local binary incompatible with the test-staking-spec.json. They have different WASM blobs (frameSystem code field).

@joelamouche
Copy link
Contributor Author

@4meta5 Good point! let me know if that's the right code

@4meta5
Copy link
Contributor

4meta5 commented Mar 18, 2021

I'm still getting the following error for reading SelectedCandidates storage item.

Trace: TypeError: Cannot read property 'toLowerCase' of undefined
    at test (/Users/4meta5/ong/ps/moonbeam-chain2/tools/test-staking.ts:63:33)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
    at process.<anonymous> (/Users/4meta5/ong/ps/moonbeam-chain2/tools/node_modules/polkadot-launch/dist/src/index.js:186:17)
    at process.emit (events.js:326:22)
    at process.EventEmitter.emit (domain.js:486:12)
    at process.emit (/Users/4meta5/ong/ps/moonbeam-chain2/tools/node_modules/source-map-support/source-map-support.js:495:21)
    at processPromiseRejections (internal/process/promises.js:245:33)
    at processTicksAndRejections (internal/process/task_queues.js:94:32)
Trace: TypeError: Cannot read property 'toLowerCase' of undefined
    at test (/Users/4meta5/ong/ps/moonbeam-chain2/tools/test-staking.ts:63:33)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
    at process.<anonymous> (/Users/4meta5/ong/ps/moonbeam-chain2/tools/node_modules/polkadot-launch/dist/scripts_moonbeam/test_multi_node_transfers.js:159:17)
    at process.emit (events.js:326:22)
    at process.EventEmitter.emit (domain.js:486:12)
    at process.emit (/Users/4meta5/ong/ps/moonbeam-chain2/tools/node_modules/source-map-support/source-map-support.js:495:21)
    at processPromiseRejections (internal/process/promises.js:245:33)
    at processTicksAndRejections (internal/process/task_queues.js:94:32)
^CSIGINT test
exit index test

Also, we should add these tests passing to the CI. The CI isn't telling us anything about these tests now so it is misleading.

@joelamouche
Copy link
Contributor Author

Also, we should add these tests passing to the CI

That is not planned yet. What do you think @crystalin

@joelamouche
Copy link
Contributor Author

@4meta5 can you try it again? it works for me now

@4meta5
Copy link
Contributor

4meta5 commented Mar 19, 2021

@joelamouche It works now for me, I think this can be merged.

@4meta5 4meta5 merged commit d687155 into master Mar 19, 2021
@4meta5 4meta5 deleted the antoine-test-staking branch March 19, 2021 01:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A0-pleasereview Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low Does not elevate a release containing this beyond "low priority".
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants