Staking support #99

Demi-Marie · 2020-04-30T22:22:44Z

This is incomplete, but I would still like a review.

dvc94ch · 2020-05-01T15:57:18Z

I think it might be better to split the crate in to core and use the proc macros for deriving the boilerplate, as it's going to be a lot for a module like staking...

Demi-Marie · 2020-05-01T17:55:09Z

@dvc94ch I was not aware that there were macros available, thanks! Some of the struct and type definitions were copied verbatim from the staking module in the runtime, but I can switch to using the runtime crate from crates.io instead.

ascjones · 2020-05-12T11:26:18Z

proc macro split is merged now so can try it out here

Demi-Marie · 2020-05-14T23:43:47Z

The proc macros seem broken. At the very least, I could not use them to obtain the validator set.

dvc94ch · 2020-05-14T23:45:06Z

Can you elaborate? What does cargo expand say? What exactly did you try?

ascjones · 2020-05-15T09:35:12Z

@demimarie-parity presumably #111 is what you mean

Demi-Marie · 2020-05-15T17:16:20Z

@ascjones indeed it is.

Demi-Marie · 2020-05-15T19:55:07Z

The proc macros are now fixed.

This currently fails with compilation errors I do not understand.

This fixes a compile error

src/frame/staking.rs

DemiMarie · 2020-09-17T15:59:27Z

@dvc94ch can we go ahead and merge this as-is? This is probably the last remaining blocker for Ledgeracio.

DemiMarie · 2020-09-17T16:00:29Z

Also @ascjones @dvdplm.

dvdplm · 2020-09-17T16:41:48Z

src/frame/staking.rs

+}
+
+#[cfg(all(test, feature = "integration-tests"))]
+mod tests {


Please clean up the tests before asking for a review. This is PoC-level stuff here. :)

The integration-tests feature is necessary because the tests don’t work without a running Polkadot and/or Kusama node. I could have written

#[cfg(test)] #[cfg(feature = "integration-tests")]

or added #[cfg(feature = "integration-tests")] on each test, but this is equivalent and shorter.

I think he is referring to looking how it is handled already by other tests. Even better would be to add the staking module to the test runtime we use for tests

I tried adding the staking module, but it would up pulling in a huge portion of Kusama. I think we would be better off using an actual Kusama node instead.

Is there a reason to use a feature gate instead of #[ignore]?

I also fixed the test suite to retry on temporary errors.

No I was referring to the test code itself: it contains a bunch of my lame attempts at figuring out how to test this. It's not something we want to actually review. :/

I think @ascjones added the integration-tests feature as a way to (eventually) run these tests in CI. There's a problem though: the contracts tests he added in #163 assumes the "remote" node has the contracts pallet, which is true for a substrate node, but isn't for polkadot/kusama. This means cargo test --feature integration-tests will pass or fail tests depending on which flavour of node you're running. :/

@demi the retry thing doesn't actually work for me. If you use _and_watch() and check on what the outcome actually is you'll see that it's not ok.

EDIT: I think the problem is that if you run a single node the chain isn't progressing so when we submit several extrinsics they end up failing in random order with "Extrinsic Usurped" or "Priority is too low: (1155024648500 vs 1155024648500)".

Edit-2: the contracts tests are run with the test-node but they keep failing for me so I must be doing something wrong.

EDIT: I think the problem is that if you run a single node the chain isn't progressing so when we submit several extrinsics they end up failing in random order with "Extrinsic Usurped" or "Priority is too low: (1155024648500 vs 1155024648500)"

Yeah usually that is probably because the nonce needs incrementing. I came up with a rudimentary solution here which appears to work consistently: https://github.com/paritytech/substrate-subxt/blob/aj-contract-call/src/frame/contracts.rs#L175

I tried adding the staking module, but it would up pulling in a huge portion of Kusama. I think we would be better off using an actual Kusama node instead.

I agree. I assume these tests would work against node-runtime too? If so then #169 might be a solution.

That said for this PR I am happy for the tests to be run manually against a local node of whatever flavour.

This reverts commit bc8bc36.

Merge branch 'staking' of github.com:paritytech/substrate-subxt into staking * 'staking' of github.com:paritytech/substrate-subxt: Ignore the staking tests on CI Retry on temporary failures Revert "Remove unnecessary use" Remove unnecessary use Add missing docs Add SetCodeWithoutChecksCall (#166) Test chill Remove dead code Hopefully fix CI

dvdplm

I think this is good enough. The tests are imperfect but I do not think this is the PR where we fix the tricky problem of solid integration testing and state setup.

ascjones

Looks good, just the kusama feature needs removing I think

ascjones · 2020-09-21T09:47:19Z

Cargo.toml

@@ -17,6 +17,8 @@ keywords = ["parity", "substrate", "blockchain"]
 include = ["Cargo.toml", "src/**/*.rs", "README.md", "LICENSE"]

 [features]
+kusama = []
+default = ["kusama"]


Why is kusama the default feature?

ascjones · 2020-09-21T11:42:15Z

src/runtimes.rs

+}
+
+/// Parachain marker struct
+#[cfg(feature = "kusama")]


Seems like this is used below without the feature flag so compilation might fail without the "kusama" feature?

ascjones · 2020-09-22T09:34:12Z

README.md

@@ -6,6 +6,9 @@ A library to **sub**mit e**xt**rinsics to a [substrate](https://github.com/parit

 See [examples](./examples).

+If you use `#[derive(Call)]` without `#[module]` in the same module, you will get errors
+complaining about an undefined method with a name starting with `with_`.


I've raised an issue for it to improve the error message: #170. But probably we should remove from the readme since it doesn't make sense without any context.

ascjones · 2020-09-22T09:45:34Z

src/frame/system.rs

@@ -101,6 +101,8 @@ pub trait System {
        + MaybeSerialize
        + Debug
        + MaybeDisplay
+        + Encode
+        + Decode


I think these are redundant, they come with Paramater

q: Parameter also comes with fmt::Debug, so should I drop that too?

* Address review grumbles from #99 * obey the fmt

Demi-Marie added 4 commits April 30, 2020 18:15

Initial Staking API

6bdf5e8

Merge branch 'master' of github.com:paritytech/substrate-subxt

10667e3

Add more staking types

d4a085a

Reformat

c08e22a

Demi-Marie assigned dvc94ch May 1, 2020

dvc94ch removed their assignment May 1, 2020

Demi-Marie added 4 commits May 1, 2020 15:31

Remove dead code

93c347c

Fix missing documentation

fef1260

Reformat

3d6e2ee

Merge branch 'master' into staking

c0b1904

Merge branch 'master' into staking

8721237

Demi-Marie added 7 commits May 15, 2020 16:25

Staking: use proc macros

a28fb20

Add partial session support

cfabfe8

Reformat

ff967e9

Try to implement nomination

716dd13

This currently fails with compilation errors I do not understand.

Use the #[module] macro

88ac3cb

This fixes a compile error

Explain undefined method diagnostics

122260f

Use ‘#[module]’ and implement session for Kusama

ffbffd4

dvc94ch reviewed May 17, 2020

View reviewed changes

src/frame/staking.rs Outdated Show resolved Hide resolved

Demi-Marie added 3 commits May 17, 2020 15:50

Don’t impl ‘Staking’ for all ‘T: System’

1b4dc07

Add staking payout support

ebfdc17

Fix compilation errors and remove useless lifetimes

aaf190e

Test chill

b54b1b9

DemiMarie requested review from dvdplm and ascjones September 17, 2020 15:59

dvdplm reviewed Sep 17, 2020

View reviewed changes

DemiMarie added 6 commits September 17, 2020 13:15

Add missing docs

68ff645

Merge remote-tracking branch 'origin/master' into staking

9c6c663

Remove unnecessary use

bc8bc36

Revert "Remove unnecessary use"

dd4dce9

This reverts commit bc8bc36.

Retry on temporary failures

1eb67d9

Ignore the staking tests on CI

d63661f

DemiMarie requested review from dvdplm and DemiMarie September 17, 2020 21:49

dvdplm and others added 10 commits September 18, 2020 13:14

Obey the fmt

24848b3

Run CI with at most one test thread

e2862d8

Implement tests for staking

8a79571

More tests

f3da309

Remove unhelpful println!

18e8029

Revert changes in contract tests

f7cda4a

Reformat

15c0206

Remove spurious diff

d659e91

More tests

52960b7

dvdplm approved these changes Sep 21, 2020

View reviewed changes

DemiMarie merged commit f9f69b8 into master Sep 21, 2020

DemiMarie deleted the staking branch September 21, 2020 17:15

ascjones reviewed Sep 22, 2020

View reviewed changes

dvdplm added a commit that referenced this pull request Sep 22, 2020

Address review grumbles from #99

f89e951

ascjones pushed a commit that referenced this pull request Sep 22, 2020

Address review grumbles from #99 (#171)

56bd633

* Address review grumbles from #99 * obey the fmt

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Staking support #99

Staking support #99

Demi-Marie commented Apr 30, 2020

dvc94ch commented May 1, 2020

Demi-Marie commented May 1, 2020

ascjones commented May 12, 2020

Demi-Marie commented May 14, 2020

dvc94ch commented May 14, 2020

ascjones commented May 15, 2020

Demi-Marie commented May 15, 2020

Demi-Marie commented May 15, 2020

DemiMarie commented Sep 17, 2020

DemiMarie commented Sep 17, 2020

dvdplm Sep 17, 2020

DemiMarie Sep 17, 2020

dvc94ch Sep 17, 2020

DemiMarie Sep 17, 2020

dvc94ch Sep 17, 2020

DemiMarie Sep 17, 2020

DemiMarie Sep 17, 2020

dvdplm Sep 18, 2020 •

edited

Loading

ascjones Sep 21, 2020

dvdplm left a comment

ascjones left a comment

ascjones Sep 21, 2020

ascjones Sep 21, 2020

ascjones Sep 22, 2020

ascjones Sep 22, 2020

dvdplm Sep 22, 2020

Staking support #99

Staking support #99

Conversation

Demi-Marie commented Apr 30, 2020

dvc94ch commented May 1, 2020

Demi-Marie commented May 1, 2020

ascjones commented May 12, 2020

Demi-Marie commented May 14, 2020

dvc94ch commented May 14, 2020

ascjones commented May 15, 2020

Demi-Marie commented May 15, 2020

Demi-Marie commented May 15, 2020

DemiMarie commented Sep 17, 2020

DemiMarie commented Sep 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvdplm Sep 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvdplm left a comment

Choose a reason for hiding this comment

ascjones left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvdplm Sep 18, 2020 •

edited

Loading