quarkchain / quarkchain-contracts Goto Github PK

Looks like the amount parameter can be one of the following:
1, the native token amount, i.e., gasPrice * gasUsed (or start)
2, the native token gasPrice.

If it works for both, I think we need to at least add one comment to clarify.

It is better to use nonReentrant modifier for withdrawStakes, withdrawMinerReward, and
transferMaintainerFee

We add OVERTIME constant if there is a bid that is close to the end. E.g., there are 2 mins left, then it will add 2 + 5 mins to 7 mins. However, I think it may better if there are < 5 mins left for a bid, we just set the end time so that only 5 mins left.

Looks like the previous auction's owner will be overridden to address(0).

Sometimes it is converted to uint64 while sometimes uint128

If an attacker intentionally withdraw a 0 balance stake where the stake's address is not in the array, I think an unexpected result will happen

because of PoSW lockup, it's possible that malicious miner will keep mining and not release stakers' funds. having a pool admin who can adjust the fee rate can mitigate this problem:

1. a staker deploys the contract and sets the pool admin as himself/herself, then find other stakers and a miner to join. if the miner does't want to release the stakes, the pool admin should adjust the fee rate to 0 such that the miner won't get block rewards going forward; if the pool admin has bad intentions and sets the rate to an unreasonably high value, the miner can withdraw the hash power at any time. note that social consensus is needed if any staker wants to withdraw stakes during the mining, and it's up to the miner and stakes to discuss how the process should go (e.g. can withdraw every week / month while the miner stops mining temporarily)
2. on the other hand, the miner can deploy the contract setting the pool admin to zero address, which means the fee rate is fixed. then the stakers must trust the miner to be able to withdraw the stakes

The logic is the same and we should reuse the logic.

We use uint16 to enumerate all stakers, which is up to 65535, while maximum stakers is uint128

One is > while another is >=.

instead of explicitly calling start and end, we want to have a more autonomous approach

1. supervisor (could be DAO in the future) sets the parameters
2. the first bidder's action (bid) will automatically start the auction process, until duration is reached
3. once ended, a new bid or an explicit call to end the auction will finalize the results of last round of auction

after merging #9, unit tests are needed

After the expiration time, the miner would get no profits. He must stop mining and let stakers withdraw their coins.

The endTime calculation is repeatedly used in several places, and they should be either replaced by an internal function or a field in auction directly.

Such as 10%.

This prevents a bidder submitting a bid and then refund most of the QKC via a large gas transaction with a negligible cost.

since we can use the default paused variable to disallow other shards' auction, we don't need a separate allowMint parameter

The minimum increment should be the factor * current_price, e.g., at least 5% greater than the current price.

non-reserved tokens are generated from the auction, and we may want to limit of the length of those token IDs

Since we stopping withdrawing the balance of the highest bidder, we internally will make sure that the equation is true and thus "require" should be replaced by "assert" (require will be only used to check the inputs are valid, while assert is used to check whether the internal data structure is correct).

SafeMath already has an overflow check so we don't need the require's manually.

The owner may give up its ownership by setting the owner as 0x0, which means it can be bid again given this line.

I would suggest adding a field in the map to indicate whether the token is taken or not.

To avoid malicious attacks.

...such that the a certain portion of miner rewards should go to the pool

unit tests should be under test folder

for auctioning new tokens:

1. start an auction, one person bids, auction ends, he wins
2. start an auction, one person bids, another person bids with a lower price, should fail
3. start an auction, one person bids, another person outbids auction ends, the other guy wins
4. start an auction, one person bids, another person places a bid at last minute before end time, the auction is extended. When the auction ends, the last bidder wins.
5. (more to come)

for gas reserves:

1. first time adding reserve should succeed
2. adding more QKC to the reserve should succeed
3. another person requests to have a higher exchange rate, should succeed
4. another person requests to have a higher exchange rate, but doesn't have any QKC, should fail
5. (more to come)

for how to write contract tests, this is a working example

feel free to add other test cases
cc @qizhou

I see a lot of conversion from uint256 to uint128. Although it should be generally safe to convert as we have up to 1e28 tokens (about 2**93), but the conversion will succeed even such conversion may result in data loss.

Notice that changing to uint256 will add extra gas cost to adding and withdrawing stakes (I think it is about 5000 or 20000).

To help stakers to contact the miner or admin when they need to withdraw or arbitrate.