Skip to content
This repository has been archived by the owner on Mar 23, 2023. It is now read-only.

fix: don't manipulate raw transaction when it is stored #1747

Merged
merged 10 commits into from
Mar 10, 2020
Merged

fix: don't manipulate raw transaction when it is stored #1747

merged 10 commits into from
Mar 10, 2020

Conversation

dated
Copy link
Contributor

@dated dated commented Mar 1, 2020 •
edited
Loading

Summary

Fixes #1746 by not manipulating the raw transaction.

Checklist

  • Documentation (if necessary)
  • Tests (if necessary)
  • Ready to be merged

@ghost
Copy link

ghost commented Mar 1, 2020

Thanks for submitting this pull request! A maintainer will review this in the next few days and explicitly select labels so you know what's going on.

If no reviewer appears after a week, a reminder will be sent out.

@ghost ghost added Complexity: Low Less than 64 lines changed. Type: Bugfix The pull request fixes an incorrect functionality or behaviour. labels Mar 1, 2020
@alexbarnsley alexbarnsley added the Bounty: Tier 4 Awarded for small features, refactorings, improvements. This is valued at 20 USD. label Mar 1, 2020
@ghost ghost mentioned this pull request Mar 1, 2020
Closed
@dated dated mentioned this pull request Mar 2, 2020
Closed
@alexbarnsley
Copy link
Member

alexbarnsley commented Mar 3, 2020 •
edited
Loading

Could you add test(s) for this please @dated

@dated
Copy link
Contributor Author

dated commented Mar 4, 2020

Are you satisfied, @alexbarnsley?

@alexbarnsley
Copy link
Member

It's a good start :trollface:

@dav1app dav1app self-requested a review March 9, 2020 17:40
dav1app
dav1app approved these changes Mar 9, 2020
View reviewed changes
Copy link
Contributor

@dav1app dav1app 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 that we should also use Date.now() from now on. Just in case anyone is wondering why.

@ghost ghost added the Status: Contributor Approved The pull request has been approved by a contributor. label Mar 9, 2020
@ghost
Copy link

ghost commented Mar 9, 2020

A contributor has approved this PR. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait.

Thank you for your contribution!

@dated
Copy link
Contributor Author

dated commented Mar 9, 2020

I think that we should also use Date.now() from now on. Just in case anyone is wondering why.

Yep that's exactly why I changed it here. getTime() only makes sense if you need to reference the same Date instance multiple times.

@faustbrian faustbrian merged commit 191de53 into ArkEcosystem:develop Mar 10, 2020
@ghost
Copy link

ghost commented Mar 10, 2020

Your pull request has been merged and marked as tier 4. It will earn you $20 USD.

@dated dated deleted the fix/tx-resend-timestamp branch March 10, 2020 04:03
@ghost ghost mentioned this pull request Mar 15, 2020
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dav1app dav1app dav1app approved these changes

@alexbarnsley alexbarnsley Awaiting requested review from alexbarnsley

@faustbrian faustbrian Awaiting requested review from faustbrian

@ItsANameToo ItsANameToo Awaiting requested review from ItsANameToo ItsANameToo is a code owner

@luciorubeens luciorubeens Awaiting requested review from luciorubeens

Assignees
No one assigned
Labels
Bounty: Tier 4 Awarded for small features, refactorings, improvements. This is valued at 20 USD. Complexity: Low Less than 64 lines changed. Status: Contributor Approved The pull request has been approved by a contributor. Type: Bugfix The pull request fixes an incorrect functionality or behaviour.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bug: resending transactions fails without error
5 participants
@dated @alexbarnsley @dav1app @faustbrian @brenopolanski