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.

Sign up for GitHub

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

Jump to bottom

GRPH-56 Implement transaction size check (18.04) #72

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

GRPH-56 Implement transaction size check (18.04) #72

wants to merge 4 commits into from

Conversation

ronakpatel3110
Copy link

No description provided.

@bobinson bobinson added the hardfork Needs hardfork label Aug 27, 2019
@bobinson bobinson mentioned this pull request Aug 27, 2019
Closed
@bobinson
Copy link

bobinson commented Sep 8, 2019

@ronakpatel3110 : Can you provide steps to test this ? The QA team will need specific steps to reproduce and test the scenario.

@bobinson bobinson added Changes Requested Changes Requested via review security security and stablity fixes labels Sep 8, 2019
@bobinson bobinson self-requested a review September 8, 2019 18:47
bobinson
bobinson requested changes Sep 8, 2019
View reviewed changes
Copy link

@bobinson bobinson left a comment

Choose a reason for hiding this comment

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

  • rebase and send the PR to develop
  • provide detailed (as detailed as possible) for the QA team to fix it.

@ronakpatel3110 ronakpatel3110 changed the base branch from qa_gpos_18.04 to develop September 9, 2019 05:18
@ronakpatel3110 ronakpatel3110 force-pushed the GRPH-56-implement-tx-size-check-18.04 branch from ee14f4d to b93101c Compare September 9, 2019 05:21
@ronakpatel3110
Copy link
Author

@bobinson 1) Rebase is done and sent PR to develop 2) It is not possible for QA to check this manually as we need to push many number of operations to reproduce this issue to increase the transaction size. To test this change, a new testcase is added broadcast_transaction_too_large.

In this test case, we are pushing multiple operations by below loop:

transfer_operation trans;
trans.from = cid_id;
trans.to   = account_id_type();
trans.amount = asset(1);
for(int i = 0; i < 250; ++i )
  trx.operations.push_back( trans );

@bobinson bobinson self-requested a review September 9, 2019 11:39
bobinson
bobinson approved these changes Sep 9, 2019
View reviewed changes
Copy link

@bobinson bobinson left a comment

Choose a reason for hiding this comment

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

LGTM

@bobinson bobinson added ready2merge PRs reviewed and ready for merge, but not merged due to HF or other dependencies and removed Changes Requested Changes Requested via review labels Sep 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bobinson bobinson bobinson approved these changes

Assignees
No one assigned
Labels
hardfork Needs hardfork ready2merge PRs reviewed and ready for merge, but not merged due to HF or other dependencies security security and stablity fixes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ronakpatel3110 @bobinson