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

refactor(core-database-postgres): add nonce column #2844

Merged
merged 19 commits into from Aug 6, 2019
Merged

Conversation

vasild
Copy link
Contributor

@vasild vasild commented Jul 26, 2019

A summary of what changes this PR introduces and why they were made.

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Refactor
  • Performance
  • Tests
  • Build
  • Documentation
  • Code style update
  • Continuous Integration
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes
  • No

Does this PR release a new version?

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

  • It's submitted to the develop branch, not the master branch
  • All tests are passing
  • New/updated tests are included

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

@ghost
Copy link

ghost commented Jul 26, 2019

Your pull request doesn't follow our contribution guidelines. Please review and correct it.

@vasild vasild changed the base branch from master to 2.6 July 26, 2019 12:43
@lgtm-com
Copy link

lgtm-com bot commented Jul 26, 2019

This pull request introduces 1 alert when merging 4ec0d7f into 757b7f5 - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

@spkjp
Copy link
Contributor

spkjp commented Jul 26, 2019

The migration also needs to be executed.

@spkjp
Copy link
Contributor

spkjp commented Jul 30, 2019

The nonce also needs to be added to the database model here: https://github.com/ArkEcosystem/core/blob/2.6/packages/core-database-postgres/src/models/transaction.ts#L7

…o-db

* ArkEcosystem/core/2.6:
  test(core-transactions): initial tests for transactions handlers bootstrap methods (#2843)
  fix: multipayment balance / vote balance (#2838)
@vasild
Copy link
Contributor Author

vasild commented Jul 30, 2019

The nonce also needs to be added to the database model here: https://github.com/ArkEcosystem/core/blob/2.6/packages/core-database-postgres/src/models/transaction.ts#L7

Done: f5fc1a5

vasild and others added 9 commits July 30, 2019 15:10
* Migrate existent v1 transactions by setting their nonce accordingly
* Set a trigger to transparently set the nonce field on newly inserted
  v1 transactions
…o-db

* ArkEcosystem/core/2.6:
  refactor(core-state): expose current block for transaction handlers (#2856)
…o-db

* ArkEcosystem/core/2.6:
  release: 2.5.0-next.10 (#2852)
  fix(core-p2p): ensure payload database exists (#2851)
  release: 2.5.14
  release: 2.5.14 (#2849)
  fix(core-p2p): peer discovery limit (#2850)
  perf(core-p2p): improve transactions endpoint (#2848)
  fix(core-api): internal server error caused by invalid orderBy field (#2847)
  refactor(core-p2p): increase network timeouts (#2828)
  fix(core-api): return data directly if cache is disabled (#2831)
  fix(core-utils): add content-type header (#2840)
  perf(core-database): lookup delegates by key (#2837)
…o-db

* ArkEcosystem/core/2.6:
  build(crypto): configure rollup (#2853)
  feat: transaction type dependencies (#2859)
@faustbrian faustbrian changed the base branch from 2.6 to develop August 6, 2019 07:44
…o-db

* ArkEcosystem/core/2.6:
  refactor(core-transactions): make handler functions asynchronous (#2865)
  fix: merge
  fix: delete existing db (#2864)
  fix: clone webhook before mutating it (#2863)
  fix: delete existing db
  fix: clone webhook before mutating it
  fix(core-state): differentiate between wallets and delegates (#2854)
  fix(core-state): differentiate between wallets and delegates (#2854)
  refactor: increase transaction type size (#2861)
@vasild vasild marked this pull request as ready for review August 6, 2019 11:27
@vasild vasild requested a review from air1one as a code owner August 6, 2019 11:27
@vasild
Copy link
Contributor Author

vasild commented Aug 6, 2019

The only remaining failure is:
https://circleci.com/gh/ArkEcosystem/core/55101?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

 FAIL  __tests__/functional/transaction-forging/vote.test.ts (87.167s)
  Transaction Forging - Vote
    ✕ should broadcast, accept and forge it [Signed with 1 Passphase] (18076ms)
    ✓ should broadcast, accept and forge it [Signed with 2 Passphrases] (27062ms)
    ✓ should broadcast, accept and forge it [3-of-3 multisig] (36104ms)

  ● Transaction Forging - Vote › should broadcast, accept and forge it [Signed with 1 Passphase]

    expected dc4b8fe0c138d55a4d977904a9d5860f5b8d71782c94b78f15271c27b71b4fb6  to be forged

      27 |         await expect(transactions).toBeAccepted();
      28 |         await support.snoozeForBlock(1);
    > 29 |         await expect(transactions.id).toBeForged();
         |                                       ^
      30 |     });
      31 | 
      32 |     it("should broadcast, accept and forge it [Signed with 2 Passphrases]", async () => {

      at Object.it (__tests__/functional/transaction-forging/vote.test.ts:29:39)

That test passes when I run it locally:

PASS __tests__/functional/transaction-forging/vote.test.ts (84.364s)
  Transaction Forging - Vote
    ✓ should broadcast, accept and forge it [Signed with 1 Passphase] (18083ms)
    ✓ should broadcast, accept and forge it [Signed with 2 Passphrases] (27055ms)
    ✓ should broadcast, accept and forge it [3-of-3 multisig] (36087ms)

I think this test fails sporadically and its failure is not related to the patch in this pull request.

@spkjp spkjp changed the title Add nonce to db refactor(core-database-postgres): add nonce column Aug 6, 2019
@spkjp spkjp merged commit 79dcbe0 into develop Aug 6, 2019
@ghost ghost deleted the add-nonce-to-db branch August 6, 2019 23:35
@ghost ghost removed the Status: In Progress label Aug 6, 2019
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