[WIP] Add transaction fee #25
Conversation
|
Can we get unit test coverage of each of the particular calls confirming that fees are taken and that they come through on the transactions. |
xtc/src/fee.rs
Outdated
| @@ -0,0 +1,9 @@ | |||
| /// Compute the fee that should be taken for a transaction based on the total amount | |||
There was a problem hiding this comment.
I think the comment above kind of contradicts the implementation which doesn't take the amount into account :)
I think it is a good idea to use a fixed amount as you did, but maybe we can improve on it.
We could do something like this:
fee = min(max(2_000_000_000, amount * X%), 10_000_000_000)
so if someone transfers a larger amount that he pays more, but there is a cap of 10_000_000_000. This cap would encourage people who want to transfer let's say 10^16 cycles to use only 1 transaction instead of breaking it up to multiple smaller transactions.
regarding floating point calculations(* X%), web assembly is using an IEEE standard, so regardless of the architecture of the IC nodes, the result is the same on all nodes
https://webassembly.github.io/spec/core/syntax/types.html
xtc/src/ledger.rs
Outdated
| @@ -194,10 +198,12 @@ async fn burn(args: BurnArguments) -> Result<TransactionId, BurnError> { | |||
| Ok(()) => { | |||
| let refunded = api::call::msg_cycles_refunded(); | |||
| let cycles = args.amount - refunded; | |||
| let new_fee = compute_fee(cycles); | |||
There was a problem hiding this comment.
nice logic, I was just thinking to make the variable names a little bit more descriptive for example:
fee => fee_deducted
new_fee = actual_fee
xtc/src/stats.rs
Outdated
| @@ -19,8 +33,9 @@ pub struct StatsData { | |||
|
|
|||
| impl Default for StatsData { | |||
There was a problem hiding this comment.
hmmm, wouldn't the following code do the same?
#[derive(Default)]
struct StatsData{...}
xtc/src/ledger.rs
Outdated
| let ledger = storage::get_mut::<Ledger>(); | ||
|
|
||
| ledger | ||
| .withdraw(&user, args.amount) | ||
| .withdraw(&user, args.amount + fee) |
There was a problem hiding this comment.
Is there a particular reason why the withdraw doesn't return the 'TransferError::InsufficientBalance'? I see this .map_err(|_| TransferError::InsufficientBalance) in almost all places when we call the withdraw method.
There was a problem hiding this comment.
Yes, because TransferError is not always what we return, look at cycles_wallet.rs file, there we want to have send error as a string.
But we should refactor it to send WithdrawError and take overflows into account in that level.
xtc/src/cycles_wallet.rs
Outdated
| let ledger = storage::get_mut::<Ledger>(); | ||
| ledger | ||
| .withdraw(&user, args.cycles) | ||
| .withdraw(&user, args.cycles + fee) |
There was a problem hiding this comment.
☠️
this line looks innocent, but it is phrone to integer overflow and could result of Dank being hacked. If an attacker sends args.cycles = uint64.max - 2000, then args.cycles + fee will overflow and will be 0, so the attackers balance doesn't decrease and later he will receive uint64.max - 2000.
Of course it is unlikely that our canister have that many cycles on it (impossible right now thanks to the canister have a limit on the number of cycles). However we should never have integer overflows in our smart contracts. Rust have safe math functions they throw when integer overflow or underflow happens, I think we should use those.
| #[derive(Deserialize, CandidType, Clone)] | ||
| pub struct StatsData { | ||
| supply: Nat, | ||
| fee: Nat, |
There was a problem hiding this comment.
nice that you thought about version, but should we change the candid file too? If I am not mistaken adding a new field is a compatible change. "Existing result types may be changed, but only to a subtype of the previous type."
I am actually curious if a client having an older version of the candid can still deserialize the result produced by the new service. Not sure if it is worth investigating now though, as we don't have many existing clients.
There was a problem hiding this comment.
yes we should change the candid
|
we will need to think hard how we go live with this PR, as the current StatsData in the stable memory has a different format than the one this PR introduces |
Are these additions? Or a property renames/type changes? |
Fee was added to the StatsData and StatsData is stored in stable memory. We should at least test in dev to see if the new code will be able to initialize StatsData from stable memory which holds the older version of StatsData. I know there are backword compatibility guarantees with the candid interface, but not sure if there is any guarantee with regards to stable memory conversion. If depends if they serialize stucts in a descreptive way or just serialize the raw data. |
|
@ferencdg The upgrade from the stable memory is taken into account in the code. For every version we keep the data structure used in the previous version, usually append a V0 to the name. The current canister that is deployed will write the data of the format |
Pull Request Test Coverage Report for Build 1221943498
💛 - Coveralls |
This PR adds a constant transaction fee of 2B cycles for every transaction, keeps track of total volume of captured fees and handles the upgrade logic required for deployment.