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

Qtum delegation #1096

Merged
merged 64 commits into from Nov 1, 2021
Merged

Qtum delegation #1096

merged 64 commits into from Nov 1, 2021

Conversation

Milerius
Copy link

@Milerius Milerius commented Oct 5, 2021

WIP:

  • add_delegation ✅
  • remove_delegation ✅
  • rpc add_delegation ✅
  • rpc remove_delegation ✅
  • rpc get_staking_infos ✅
  • unit test transaction generation add_delegation ✅
  • unit test transaction generation remove_delegation ✅
  • unit test transaction generation proof of delegation ✅
  • unit test RPC's ✅

#802

Some info before reviewing from QTUM developers:

ROI and Fees:

The ROI for staking is available at https://qtum.info/misc/toolbox/stake-calculator. "Expected time" to a block reward is a probability term, there is much variation, but this value is an excellent long-term average.

A default super staker fee is 10%; some go lower.

Staking amount:

You can't choose the amount to delegate, only the address to delegate, and then all the UTXOs over 100.0 QTUM in that address will be staked by the super staker.

Non-custodial

The delegation is non-custodial, so you still have complete control of the funds. You can add or remove funds, and the delectation will remain. If you want to move to another super staker, then undelegate first. A UTXO that has just been staked (by the super staker) may have to wait for 2,000 confirmations before sending.

The last point is charming. This means that no modification is required regarding balance spendable or not; as far the delectation is still active, that means for every UTXO's > 100 QTUM, you will be staking.

Please read https://docs.qtum.site/en/OfflineStakingDelegation/ before reviewing.

remove_delegation example: https://testnet.qtum.info/tx/421a628e8465783a7e1ba28803b7b882e3069d1fe070fb84ae9efb5512eb2032

add_delegation example: https://testnet.qtum.info/tx/48921789c7dc3d6362421940a38b612354615351090e21095d636bb4b031782b

Comment on lines 437 to 441
amount: BigDecimal,
to_addr: Address,
fee: u64,
) -> Result<GenerateQrc20TxResult, MmError<GenerateTxError>> {
let staker_address_hex = qtum::contract_addr_from_utxo_addr(to_addr.clone());
Copy link
Author

Choose a reason for hiding this comment

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

The amount cannot be < 100, otherwise, we need to return an error.

Token::Bytes(signature.into()),
])?;

let contract_address = ethabi::Address::from_str("0000000000000000000000000000000000000086").unwrap();
Copy link
Author

Choose a reason for hiding this comment

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

Should I turn it into a constant? It's the smart contract address of the delegation for QTUM.

Comment on lines 488 to 489
println!("res: {}", hex::encode(params));
println!("script_pubkey: {}", hex::encode(script_pubkey.clone()));
Copy link
Author

Choose a reason for hiding this comment

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

remove

@Milerius Milerius marked this pull request as ready for review October 11, 2021 10:01
Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

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

Next review iteration 🙂

mm2src/coins/utxo.rs Outdated Show resolved Hide resolved
mm2src/coins/utxo/qtum.rs Outdated Show resolved Hide resolved
mm2src/mm2_tests.rs Outdated Show resolved Hide resolved
mm2src/coins/utxo/qtum.rs Outdated Show resolved Hide resolved
Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

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

One more review iteration.

mm2src/coins/lp_coins.rs Outdated Show resolved Hide resolved
mm2src/coins/utxo/qtum.rs Outdated Show resolved Hide resolved
mm2src/coins/utxo/qtum.rs Outdated Show resolved Hide resolved
mm2src/coins/utxo/qtum_delegation.rs Show resolved Hide resolved
@@ -4,6 +4,7 @@
/// backwards compatibility
/// Use `#[serde(deny_unknown_fields)]` for all structs for tests to fail in case of adding new fields to the response
use bigdecimal::BigDecimal;
use coins::TransactionType;
Copy link
Member

Choose a reason for hiding this comment

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

Please declare the TransactionType separately here. If we occasionally make an incompatible change, we won't catch it because we import the updated struct.

mm2src/coins/utxo/qtum_delegation.rs Outdated Show resolved Hide resolved
mm2src/coins/utxo/qtum_delegation.rs Outdated Show resolved Hide resolved
Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

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

Few minor nitpicks 🙂

mm2src/coins/qrc20/rpc_clients.rs Outdated Show resolved Hide resolved
mm2src/coins/qrc20/rpc_clients.rs Outdated Show resolved Hide resolved
mm2src/coins/utxo/qtum.rs Show resolved Hide resolved
artemii235
artemii235 previously approved these changes Oct 27, 2021
Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

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

🔥

Copy link

@sergeyboyko0791 sergeyboyko0791 left a comment

Choose a reason for hiding this comment

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

Everything is almost perfect. Could you please fix the last few comments?

@@ -442,6 +472,41 @@ impl WithdrawRequest {
}
}

/// Please note that no type should have the same structure as another type,
/// because this enum has the `untagged` deserialization.

Choose a reason for hiding this comment

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

There is the comment yet :D

because this enum has the untagged deserialization.

.map_to_mm(|e| StakingInfosError::Transport(e.to_string()))?;
let am_i_staking = add_delegation_history.len() > remove_delegation_history.len();
if am_i_staking {
let last_tx_add = &add_delegation_history[add_delegation_history.len() - 1];

Choose a reason for hiding this comment

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

It shouldn't crush since add_delegation_history.len() > remove_delegation_history.len() is true if add_delegation_history is not empty. But could you please add an explicit check for the length of add_delegation_history, or use

let last_tx_add = match add_delegation_history.last() {
  Some(last_tx_add) => last_tx_add,
  None => return Ok(None),
};

I ask you to do it because someone, including me, can change the if condition and forget to check the length.

// topic[0] -> a23803f3b2b56e71f2921c22b23c32ef596a439dbe03f7250e6b58a30eb910b5
// topic[1] -> 000000000000000000000000d4ea77298fdac12c657a18b222adc8b307e18127 -> staker_address
// topic[2] -> 0000000000000000000000006d9d2b554d768232320587df75c4338ecc8bf37d
let raw = res[0].log[0].topics[1].trim_start_matches('0');

Choose a reason for hiding this comment

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

AddDelegation contract call can be not only the first output of a transaction, but also second and third, and so on. Please look at the example. You can check which output emitted QTUM_ADD_DELEGATION_TOPIC something like this:

let receipt = res.into_iter().find(|receipt| receipt.log.iter().any(it_qtum_add_delegation)).expect("TODO using match");

Choose a reason for hiding this comment

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

Now it looks perfect, thanks!

Copy link

@sergeyboyko0791 sergeyboyko0791 left a comment

Choose a reason for hiding this comment

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

Approve!!!

Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

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

@Milerius Please fix Clippy and make sure you run this check locally before pushing, especially to the almost finished PR.

Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

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

Reapproving

@artemii235 artemii235 merged commit dea71de into dev Nov 1, 2021
@artemii235 artemii235 deleted the qtum_delegation branch November 1, 2021 08:22
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

4 participants