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

[Qanet] Ensure compatibility with TFChain 2.9 release #3340

Closed
Tracked by #1565
sameh-farouk opened this issue Aug 26, 2024 · 32 comments
Closed
Tracked by #1565

[Qanet] Ensure compatibility with TFChain 2.9 release #3340

sameh-farouk opened this issue Aug 26, 2024 · 32 comments
Assignees
Milestone

Comments

@sameh-farouk
Copy link
Member

sameh-farouk commented Aug 26, 2024

Which package/s are you suggesting this feature for?

No response

Is your feature request related to a problem? Please describe

Ensure compatibility with TFChain 2.9 release / TFChain runtime v152

Describe the solution you'd like

For better context
PR: threefoldtech/tfchain#992

Relevant SDK Adjustments Needed:

Fund Reservations: The system now employs a more reliable reservation method, replacing the previous use of locks/freezes.

Balance Handling: Payments on hold are no longer considered part of the free balance. The total balance should now reflect both the reserved and free portions. Previously, the free balance was considered the total balance, with amounts reserved for future payments subtracted from it. The new approach aligns with the balances pallet terminology:

  • Total Balance: The sum of the reserved and free balances in the user's account.
  • Reserved: Funds still owned by the account holder but set aside (e.g., for future contract payments).
  • Free: The portion of the balance that is available for spending, minus the Existential Deposit, assuming no locks are present.
  • Locked: A freeze on a specific amount of the free balance, no longer used in TFChain smart contract payments.

Contract Payment Tracking: The ContractLock has been replaced with ContractPaymentState to better track and manage the states of contract payments, particularly during grace periods and when dealing with overdue payments.
https://github.com/threefoldtech/tfchain/blob/622e7d255b9c76203fba8a316b84b280b59fbba7/substrate-node/pallets/pallet-smart-contract/src/types.rs#L219-L224

https://github.com/threefoldtech/tfchain/blob/83ccaa45d86076f6730d3611ce0c826c69dce649/substrate-node/pallets/pallet-smart-contract/src/types.rs#L236-L243

Dashboard Restore Contracts Feature: After the update to the new runtime, the dashboard's "restore contracts" feature will no longer be usable. To correctly reinstate this functionality, the following steps are needed:

  • Overdraft Tracking: The system now tracks overdrafts per contract. The overdraft is updated every hour or when billing is triggered.
  • Calculating Overdue Amounts: To calculate the overdue amount, sum the overdraft in the contract payment state with the product of ((seconds elapsed since the last billing + an extra time allowance for the user to take action) * the contract cost per second). This will give the correct overdue amount that needs to be addressed.

Update:
Possibly less relevant changes::
Event Emission Changes: The events emitted from the billing process have been updated, and several changes have been made:

  • New Events:

    • ContractPaymentOverdrawn: This event is closely related to the ContractBilled event. Both events provide information about the amount due for the previous billing period. The ContractBilled event indicates that the due amount was successfully charged to the user's account. In contrast, the ContractPaymentOverdrawn event signifies that the due amount was not fully covered and has been added to the contract's overdraft. This distinction helps in accurately tracking the financial state of contracts.
    • RewardDistributed: This event signifies the successful distribution of rewards after the billing cycle, providing transparency and traceability for reward payouts.
    • ContractGracePeriodElapsed: This event is triggered when a contract has exceeded its grace period without sufficient funds, leading to its transition to a deleted state.
  • No longer emitted as part of the billContract call

    • Locked: It have been replaced with Reserved event
    • TokenBurned: The TokenBurned event is no longer emitted, reflecting the removal of TFT burning from TFChain.
    • Unlocked and Transfer Events: Both the Unlocked and Transfer events have been replaced with the ReserveRepatriated event.

New Errors:

  • RewardDistributionError
  • ContractPaymentStateNotExists
@sameh-farouk
Copy link
Member Author

sameh-farouk commented Aug 27, 2024

@AhmedHanafy725 A last-minute change that is worth highlighting.
threefoldtech/tfchain#992 (comment)

@0oM4R 0oM4R moved this from Accepted to In Progress in 3.15.x Aug 28, 2024
@0oM4R
Copy link
Contributor

0oM4R commented Aug 28, 2024

Will start with the balance changes and running the node, and will apply the contract lock changes tomorrow:
will give it 4h for both.

@0oM4R 0oM4R self-assigned this Aug 28, 2024
@0oM4R
Copy link
Contributor

0oM4R commented Aug 28, 2024

Issue update:
applied balance changes on dashboard

@0oM4R 0oM4R mentioned this issue Aug 28, 2024
5 tasks
@0oM4R
Copy link
Contributor

0oM4R commented Aug 29, 2024

Issue Update:
support getting overdue amount functions.
I faced some issues about the overdue types and typegeneration; update generatetypes script and used Decimal as we bigint supported only after ES2020

Question: in dashboard, should we go with locked contract and locked amount or paused contract and overdue contract

time estimate update: will add 2h for playground edits

@sameh-farouk
Copy link
Member Author

sameh-farouk commented Aug 30, 2024

Question: in dashboard, should we go with locked contract and locked amount or paused contract and overdue contract

For the first case, paused contract is okay, but I prefer suspended contract. Both terms are suitable.

For the second case, locked amount and overdue contract refer to different things.
If you need to reference the amount that needs to be settled before contracts can resume, you may use the term overdue amount. Alternatively, overdraft amount can be used, or maybe just overdraft as we refer to it on tfchain.

On the other hand, the locked amount is now referred to as the reserved amount and represents the amount billed but not fully processed (pending transfer to beneficiaries).

@0oM4R
Copy link
Contributor

0oM4R commented Sep 1, 2024

Issue update:
applied the changes on dashboard functions, but kept the old UI and names.
I couldn't test the pr will keep it as drafted till we have chain deployed to test the logic.

@0oM4R 0oM4R moved this from In Progress to In Verification in 3.15.x Sep 1, 2024
@0oM4R 0oM4R moved this from In Verification to Blocked in 3.15.x Sep 1, 2024
@0oM4R
Copy link
Contributor

0oM4R commented Sep 1, 2024

moved to blocked on chain deployment

@sameh-farouk
Copy link
Member Author

Any reason for keeping old names?

I understand the need for consistency to avoid confusing users, but this should not prevent us from correcting or improving some UI terms.

Referring to the amount needed to be settled as a locked amount is not correct. It's the other way around; the locked amount is the amount already settled/reserved.
Suspended or paused contracts term are much clearer than locked contracts.

@sameh-farouk
Copy link
Member Author

Just a friendly reminder, we should conduct tests locally whenever possible. Here are the steps to follow:

  • Launch a container using the latest tfchain image 2.9.0
  • Connect a local instance of the dashboard to the local tfchain node
  • over Polkadot.js, Agree to the terms and conditions, and create a twin
  • Create a name contract (as it is easier to create and doesn't require a node/farm setup)
  • After that, you can test any scenario involving billing, balances, or the grace period.

I understand the temptation to wait for the devnet deployment, but I want to highlight your options here.
I believe it would be a valuable experience to have as well (testing using local tfchain instance).

@0oM4R
Copy link
Contributor

0oM4R commented Sep 2, 2024

Any reason for keeping old names?

I understand the need for consistency to avoid confusing users, but this should not prevent us from correcting or improving some UI terms.

Referring to the amount needed to be settled as a locked amount is not correct. It's the other way around; the locked amount is the amount already settled/reserved. Suspended or paused contracts term are much clearer than locked contracts.

I'm leaving it as is until we talk to @AhmedHanafy725 . I will check with him as soon as possible and apply the changes.

@0oM4R
Copy link
Contributor

0oM4R commented Sep 2, 2024

Just a friendly reminder, we should conduct tests locally whenever possible. Here are the steps to follow:

  • Launch a container using the latest tfchain image 2.9.0
  • Connect a local instance of the dashboard to the local tfchain node
  • over Polkadot.js, Agree to the terms and conditions, and create a twin
  • Create a name contract (as it is easier to create and doesn't require a node/farm setup)
  • After that, you can test any scenario involving billing, balances, or the grace period.

I understand the temptation to wait for the devnet deployment, but I want to highlight your options here. I believe it would be a valuable experience to have as well (testing using local tfchain instance).

grid client is using graphql to generate the consumption rate, and it is used to get the overdue amount. already tested creating name contract and getting the payment state but I couldn't calculate the overdue amount to test resuming graceperiod contracts.
also the dashboard needs graphql to list the contracts, so i can't test the flow in the current situation.
if you have any suggestion about testing please let me know

@sameh-farouk
Copy link
Member Author

@0oM4R You could run a local Graphql instance that listens to the local TFChain.

@0oM4R
Copy link
Contributor

0oM4R commented Sep 2, 2024

@0oM4R You could run a local Graphql instance that listens to the local TFChain.

As discussed with Thabet will wait for the chain deployment

@sameh-farouk
Copy link
Member Author

sameh-farouk commented Sep 3, 2024

Runtime was updated on devnet

@0oM4R 0oM4R moved this from Blocked to In Progress in 3.15.x Sep 3, 2024
@0oM4R
Copy link
Contributor

0oM4R commented Sep 3, 2024

Issue update:
Applied pr comments, and working on writing the new contract coast calculation, this also needs some logic to get the required parameters in the calculation function.
will update the estimate time to be more 3 hours for implementation only

@0oM4R
Copy link
Contributor

0oM4R commented Sep 4, 2024

Issue update:

cost calculation is almost done, need to handle musd price and the node contract on rented node case.

todo:
fix broken features related to grace period in dashboard, after complete clients changes

@0oM4R
Copy link
Contributor

0oM4R commented Sep 7, 2024

Issue update:
client side changes almost complete, need to verify some calculations,
will implement the dashboard changes tmw

@0oM4R
Copy link
Contributor

0oM4R commented Sep 8, 2024

Issue update:
we need to refactor the flow of node contract on rented node to add the overdraft and nu and ipv4 costs
-- Blocked on the calculations, i don't understand the current calculations, the returned amount is not clear on which unit

@0oM4R
Copy link
Contributor

0oM4R commented Sep 8, 2024

Issue update:
prices now should be correct; the node contracts with ipv4 cost will be added to its rent contract also the un-billed NU.
will verify the domain name tmw.

dashboard now calling the new methods, need to implement the new flow of node contracts on the rent contract only;

@0oM4R
Copy link
Contributor

0oM4R commented Sep 8, 2024

unique name calculation looks fine.
To do:

  • handle the resume rent contract, this should call every node contract with ipv4 on that node
  • handle lock details for node contract on rented node, will hide the details and ask the user to unlock the rent contract instead

will update the time estimation to more 8 hours; for UI changes, test, clean up the code, and prepare the pr

@0oM4R
Copy link
Contributor

0oM4R commented Sep 9, 2024

Issue update :
all changes applied, will give it another test phase and prepare the PR

@0oM4R
Copy link
Contributor

0oM4R commented Sep 10, 2024

Issue update: while testing faced a edge case, when rent contract got billed and the node contract got crated after that, and got billed before the next billling cycle of the rent contract; in this case we have node contract on grace period, and its rent contract is on created state.
work completed:

  • handled this case in grid client, if you pass the contract you will have a result no matter what type of contract you passed, if it is cost is zero 'like node contract without ipv4 or network' will have the cost equals zero, if it have overdratf or nu you will have it.
  • add docstring to the functions

todo: handle the mentioned case in UI, and test it. will take about 3h
note: testing cycle is very long, creating contracts and move them to grace period, and in some cases you have to wait for the next billing cycle.

@0oM4R 0oM4R moved this from In Progress to Pending Review in 3.15.x Sep 11, 2024
@0oM4R
Copy link
Contributor

0oM4R commented Sep 11, 2024

For rephrasing will do in separate issue #3400

@0oM4R
Copy link
Contributor

0oM4R commented Sep 19, 2024

need to include two things:

  • on node contracts on certified nodes we should add %25 of the nu,
  • we should add the extra fee to the rent contract

@0oM4R 0oM4R moved this from Pending Review to In Progress in 3.15.x Sep 19, 2024
@sameh-farouk
Copy link
Member Author

on node contracts on dedicated nodes we should add %25 of the nu,

You mean on certified nodes, not dedicated ones, right?

we should add the extra fee to the rent contract

Isn't this handled by the calc module?

@0oM4R
Copy link
Contributor

0oM4R commented Sep 30, 2024

no it is not

Isn't this handled by the calc module?

@0oM4R
Copy link
Contributor

0oM4R commented Oct 7, 2024

Issue update: while testing with Sameh we found that there is some fractions is messing, I did some refactor to increase the accuracy

  • calculate the node contract on rented contract cost, not just the ip price, we were assuming that the rent and node contracts have the same elapsed seconds
  • insure that we calculate the node contract on rented node only once in get total contracts overdue,
  • refactor the code to be in order and easy to debug
  • pass the contracts by ids in dashboard as the contract there do not have public ip field

still can't find the fractions gap, all calculation should be fine!

@0oM4R
Copy link
Contributor

0oM4R commented Oct 7, 2024

Todo dashboard: on reload contracts, the rented node ref should be cleared

@0oM4R
Copy link
Contributor

0oM4R commented Oct 8, 2024

for name contract the calculation seems to be correct and the contract moved to created state only if i used the polkadot ui and bill the contract using another account, this mean the issue might be in the extrinsic fees

@0oM4R
Copy link
Contributor

0oM4R commented Oct 8, 2024

Todo dashboard: on reload contracts, the rented node ref should be cleared

issue update : added some ui enhancements by resetting values as mentioned above

@0oM4R
Copy link
Contributor

0oM4R commented Oct 9, 2024

issue update:

  • calculate the node contract on rented contract cost, not just the ip price, we were assuming that the rent and node contracts have the same elapsed seconds
  • continue refactor the ip part, now the certification premium price is added to the ip cost
  • removed the old logic that calculates the ip price
  • while testing notice that the node 163 is diy node on gridproxy and graphql, but on the chain it is certified; this causes that we show the estimated price as the node is not certified, and the chain bill the contract as the node is certified which leads to a leak of balance and the contract will not be resumed, after mock the certification to be as the chain, the contracts resumed but we have about 3% balance left, this means we estimate a largerer value than that what actually got billed, i left the contract on node 151 that didn't got billed for more than 2 hours to check the remaining balance after resume them tomorrow

@0oM4R 0oM4R moved this from Pending Review to In Verification in 3.15.x Oct 31, 2024
@khaledyoussef24 khaledyoussef24 changed the title Ensure compatibility with TFChain 2.9 release [Qanet] Ensure compatibility with TFChain 2.9 release Nov 4, 2024
@khaledyoussef24
Copy link
Contributor

verified on qanet
Version 2.6.0-rc4

ran the billing test suite
Screenshot 2024-11-12 at 12 10 14 PM

billing is made after the first hour in the first billing cycle, determining the billing rate for each contract and deployment
Screenshot 2024-11-12 at 11 53 59 AM

when the contract is in a grace period, you can now click on the grace period and learn more about the contracts and tfts to unlock
this is for unlocking the rent node:
Screenshot 2024-11-12 at 11 58 58 AM
for unlocking the deployment :
Screenshot 2024-11-12 at 11 57 17 AM
Screenshot 2024-11-12 at 11 57 27 AM

for unlocking all contracts :
Screenshot 2024-11-12 at 11 57 44 AM

the machine on the rented went to a grace period when the node went to a grace period
Screenshot 2024-11-12 at 12 27 16 PM

tried to deploy on the node after it went grace period (was possible before)
Screenshot 2024-11-12 at 12 29 36 PM
Screenshot 2024-11-12 at 12 29 36 PM 1

unlocking contracts also works fine after funding
Screenshot 2024-11-12 at 12 34 48 PM
Screenshot 2024-11-12 at 12 34 52 PM
Screenshot 2024-11-12 at 12 34 56 PM
Screenshot 2024-11-12 at 12 35 17 PM

@khaledyoussef24 khaledyoussef24 moved this from In Verification to Done in 3.15.x Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

No branches or pull requests

6 participants
@ramezsaeed @AhmedHanafy725 @sameh-farouk @0oM4R @khaledyoussef24 and others