Skip to content

Conversation

@iadmytro
Copy link
Contributor

E2E test for rewards

Copy link
Member

@AngelCastilloB AngelCastilloB left a comment

Choose a reason for hiding this comment

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

I think this test should be located at test/wallet/SingleAddressWallet/ together with the rest of the SingleAddressWallet tests, maybe in delegation.test.ts? or in a new staking-rewards.test.ts.

@iccicci
Copy link
Collaborator

iccicci commented Sep 21, 2022

Hi @rhyslbw , @mkazlauskas , @AngelCastilloB , @mirceahasegan ;

  • the suggestions from all the conversations should be solved;
  • used TxBuilder in generateTxs as well;
  • I tried to make the flow as close as possible to the diagram Rhys provided us on slack.

Now I'm keeping my eyes on this and this CI builds to check if the test works on CI as well.

mirceahasegan
mirceahasegan previously approved these changes Sep 22, 2022
Copy link
Collaborator

@mirceahasegan mirceahasegan left a comment

Choose a reason for hiding this comment

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

Great work @iccicci and @iadmytro. 👏
The test, even though complicated, is so easy to read and a good example of how to use the SDK.

@iccicci iccicci force-pushed the test/e2e-rewards branch 3 times, most recently from c9b4c36 to 8ca0385 Compare September 26, 2022 23:29
Copy link
Member

@mkazlauskas mkazlauskas left a comment

Choose a reason for hiding this comment

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

Looking good, just a few minor improvements suggested

@iccicci
Copy link
Collaborator

iccicci commented Sep 27, 2022

Hi @rhyslbw ,

I'm sorry but I'm not able to go on this test.
I added the last step of the test as described in tickets AC (withdraw the reward) but I get an error trying to submit the withdraw tx; at least the error is deterministic and is shared in a CI workflow. I also tried to set the log level to debug as suggested by @mkazlauskas , but it seems this was generating too many logs... I had to stop CI after 3h.

This is not the only problem: once added this test to the test:wallet e2e suite (as suggested in this PR) many other tests in the same suite fail...

I tried to investigate the MissingVkWitnessesError, but I didn't understood how to handle it. I need support on this.

CC @AngelCastilloB , @mirceahasegan

extraSigner used in nft e2e test was sometimes signing with stake key,
while it should only be signing with the key used for minting
ogmios provider sometimes throws this due to websocket being disconnected
interpret UnknownOrIncompleteWithdrawalsError as tx already submitted
AngelCastilloB
AngelCastilloB previously approved these changes Oct 25, 2022
Copy link
Member

@AngelCastilloB AngelCastilloB left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Copy link
Member

@rhyslbw rhyslbw left a comment

Choose a reason for hiding this comment

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

Looks good

mkazlauskas and others added 10 commits October 25, 2022 08:28
rewards were still not generated consistently
some cleanup on delegation-rewards test
since inputs are unordered, jest would sometimes incorrectly fail the test
initializeTx awaits for epoch rollover when near it
by taking into account rewards generated

between building tx and submission confirmation
… CI not exiting

the issue most likely lies within wallet package and will be easier to reproduce
and fix in unit tests once we de-promisify SingleAddressWallet dependencies
@rhyslbw rhyslbw dismissed mkazlauskas’s stale review October 25, 2022 06:21

Martynas completed the fixes

@rhyslbw rhyslbw merged commit 26699f7 into master Oct 25, 2022
@rhyslbw rhyslbw deleted the test/e2e-rewards branch October 25, 2022 06:22
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.

7 participants