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

feat!: adding MinimumStake parameter #574

Merged
merged 8 commits into from Aug 7, 2023

Conversation

kehiy
Copy link
Contributor

@kehiy kehiy commented Jul 5, 2023

Description

This PR adds the MinimumStake parameter to the consensus parameters and the default value is set to 1 PAC coin (10^9).
Consequently, all bond transaction will reject if their value is less than the MinimumStake.

Related issue(s)

BREAKING CHANGE

@kehiy kehiy requested a review from b00f July 5, 2023 09:52
@Ja7ad
Copy link
Contributor

Ja7ad commented Jul 7, 2023

Description

I added MinimumStake amount as 1 PAC coin.
there is just one issue with the test and it is:
the random amount and fee always going to be less than MinimumStake we should change the random fee and amount or maybe the test.

Related issue(s)

Please send your PR on mainnet branch.

@kehiy
Copy link
Contributor Author

kehiy commented Jul 7, 2023

Description

I added MinimumStake amount as 1 PAC coin.
there is just one issue with the test and it is:
the random amount and fee always going to be less than MinimumStake we should change the random fee and amount or maybe the test.

Related issue(s)

Please send your PR on mainnet branch.

@Ja7ad sure!
I will send this PR and the GC pr to mainnet branch, but I should notice that this PR need a small change in merge time....

@kehiy kehiy changed the base branch from main to mainnet July 7, 2023 19:22
@kehiy kehiy changed the base branch from mainnet to main July 7, 2023 19:22
@b00f b00f marked this pull request as draft July 8, 2023 06:01
@b00f b00f changed the title feat: MinimumStake amount feat!: adding MinimumStake parameter Jul 14, 2023
types/param/param.go Outdated Show resolved Hide resolved
Co-authored-by: b00f <mostafa.sedaghat@gmail.com>
@kehiy
Copy link
Contributor Author

kehiy commented Jul 14, 2023

@b00f I commited your sugget, but don't forget to solve test issue in mergeing time.

@kehiy kehiy requested a review from b00f July 14, 2023 13:19
@b00f
Copy link
Collaborator

b00f commented Aug 6, 2023

@kehiy

I think we can fix test with small modification for randomAmountAndFee, like:

func (td *testData) randomAmountAndFee(min int64, max int64) (int64, int64) {
	amt := td.RandInt64NonZero(max)
	for amt < min {
		amt = td.RandInt64NonZero(max)
	}

	fee := int64(float64(amt) * td.sandbox.Params().FeeFraction)
	return amt, fee
}

@kehiy kehiy marked this pull request as ready for review August 6, 2023 11:30
@kehiy
Copy link
Contributor Author

kehiy commented Aug 6, 2023

@b00f some other tests going to be filled, can you review them?

@b00f
Copy link
Collaborator

b00f commented Aug 6, 2023

@kehiy Please use make test to check it locally. I can see some issues in tests.
Also, for non-bond transactions, set minimum to zero.

Thanks buddy

@kehiy
Copy link
Contributor Author

kehiy commented Aug 6, 2023

@b00f I fixed it, the last issue is pool_test.go

@kehiy kehiy requested a review from b00f August 6, 2023 16:59
@codecov
Copy link

codecov bot commented Aug 6, 2023

Codecov Report

Merging #574 (60f28bc) into main (cac4908) will decrease coverage by 0.09%.
The diff coverage is 60.00%.

❗ Current head 60f28bc differs from pull request most recent head 0e162f9. Consider uploading reports for the commit 0e162f9 to get more accurate results

@@            Coverage Diff             @@
##             main     #574      +/-   ##
==========================================
- Coverage   83.62%   83.54%   -0.09%     
==========================================
  Files         154      154              
  Lines        7267     7279      +12     
==========================================
+ Hits         6077     6081       +4     
- Misses        911      915       +4     
- Partials      279      283       +4     

execution/executor/bond_test.go Show resolved Hide resolved
execution/executor/transfer.go Show resolved Hide resolved
genesis/genesis_test.go Outdated Show resolved Hide resolved
@kehiy kehiy requested a review from b00f August 7, 2023 06:40
@themantre themantre enabled auto-merge (squash) August 7, 2023 06:52
@kehiy
Copy link
Contributor Author

kehiy commented Aug 7, 2023

@b00f checks are passed!

@themantre themantre merged commit 9569722 into pactus-project:main Aug 7, 2023
10 checks passed
@kehiy kehiy deleted the MinimumStake branch August 7, 2023 12:30
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.

Adding MinimumStake to the consensus parameters
4 participants