-
Notifications
You must be signed in to change notification settings - Fork 13
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
add DapperStorageRent to a few templates, update tests #98
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need need to add test cases for the dapperStorageRent contract and need transactions for the same as well.
@@ -0,0 +1,77 @@ | |||
import FungibleToken from "./FungibleToken.cdc" | |||
|
|||
pub contract PrivateReceiverForwarder { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why we need this contract? can't we directly transfer the funds to the address capability? because it also holds the capability for the same.
Maybe I am missing something here ?
} | ||
|
||
// 0.06 = 6MB of storage, or ~20k NBA TS moments | ||
privateForwardingSenderRef!.sendPrivateTokens(address,tokens:<-vaultRef!.withdraw(amount: REFUEL_AMOUNT)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't make sure that the user would receive the 0.06 FLOW in the given account as the user can have a different receiver capability of different accounts in the private forwarder resource.
/// @param batchSize: Int to set the batch size of the cleanup | ||
pub fun cleanExpiredRefilledAccounts(_ batchSize: Int) { | ||
var index = 0 | ||
while index < batchSize && self.RefilledAccounts.length > index { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is incorrect as it would only allow running the loop half times than the given batchSize.
If the given batchSize is 10, this loop will run 5 times because of self.RefilledAccounts.length
also decreases when you remove the index.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice catch!! working on a fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@satyamakgec updated method to the below, thoughts?
pub fun cleanExpiredRefilledAccounts(_ batchSize: Int) {
var index = 0
var refilledAccountsToCleanup: [Address] = [];
var refilledAccountsLength = self.refilledAccounts.length
while index < batchSize && index < refilledAccountsLength {
if self.refilledAccountInfos[self.refilledAccounts[index]] != nil &&
getCurrentBlock().height - self.refilledAccountInfos[self.refilledAccounts[index]]!.atBlock > self.refillRequiredBlocks {
refilledAccountsToCleanup.append(self.refilledAccounts[index])
}
index = index + 1
}
for account in refilledAccountsToCleanup {
if let idx = self.refilledAccounts.firstIndex(of: account) {
self.refilledAccounts.remove(at: idx)
self.refilledAccountInfos.remove(key: account)
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can avoid the below for
loop as we can move the removed statements of the array in the while loop. We need only these 2 statements.
var refilledAccountsLength = self.refilledAccounts.length
while index < batchSize && index < refilledAccountsLength {
} | ||
|
||
// If the user is below the threshold PrivateReceiverForwarder will send 0.06 Flow tokens for about 6MB of storage | ||
if high - low < self.StorageRentRefillThreshold { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember that the value of self.StorageRentRefillThreshold
should be greater than the txn cost of the tryRefil
transaction.
|
||
/// DapperStorageRent | ||
/// Provide a means for accounts storage TopUps. To be used during transaction execution. | ||
pub contract DapperStorageRent { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it essential to have the prefix of the name Dapper
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, not essential, but was thinking it gave some good context to its relationship to BloctoStorageRent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm.. I think it is better to remove and not use the company name as the prefix. Instead, we can go with a generic contract name to avoid some "XYZ" reasons. We can rename it to Refueler
or something like that.
@satyamakgec thanks Satyam -- i'll go through these today and get back to you with some feedback also |
DapperStorageRent -> https://github.com/dapperlabs/dapper-flow-contracts/tree/main/dapper-storage-rent
Adding
DapperStorageRent
to a few templates for later useage when we launchDapperStorageRent
.