-
Notifications
You must be signed in to change notification settings - Fork 46
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
Updated price discovery #991
base: rc/v3.0.2
Are you sure you want to change the base?
Conversation
Coverage SummaryTotals
FilesExpand
|
Contract comparison - from 58a7e74 to 6eae83b
|
|
||
```rust | ||
#[init] | ||
fn init( | ||
&self, | ||
launched_token_id: TokenIdentifier, | ||
accepted_token_id: TokenIdentifier, | ||
accepted_token_id: EgldOrEsdtTokenIdentifier, |
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.
Ideally, use EgldOrEsdtTokenIdentifier
for all args.
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.
Nah, you can't launch EGLD, so this makes it a bit more clear.
bought_token_id: EgldOrEsdtTokenIdentifier<M>, | ||
bought_token_amount: BigUint<M>, | ||
pub struct OwnerDepositEvent<'a, M: ManagedTypeApi> { | ||
token_amount_in: &'a BigUint<M>, |
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.
Lifetime of objects was hidden on purpose in the framework.
Is there a way to avoid this here?
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.
Nope. Lifetime is hidden when you call the respective helper functions, not when you define the struct.
let min_launched_tokens = self.min_launched_tokens().get(); | ||
let current_total_launched_tokens = self.launched_token_balance().get(); | ||
require!( | ||
¤t_total_launched_tokens + &payment_amount >= min_launched_tokens, |
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.
Please use the same naming for variables.
current_total_launched_tokens
, min_launched_tokens
- change this to amount;
or
payment_amount
- change this to tokens.
It's easier for the reviewer or the next developer to read and understand.
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.
Done.
|
||
fn owner_redeem(&self, owner: &ManagedAddress) -> EgldOrEsdtTokenPayment { | ||
let launched_token_supply = self.launched_token_balance().get(); | ||
require!( |
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 may be moved in the caller function redeem
.
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, I purposefully kept it in a different function.
payment_result | ||
} | ||
|
||
/// Deposit ESDT received after deposit to withdraw the initially deposited tokens. |
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.
Correct this comment.
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 already did? What were you expecting the comment to say?
|
||
self.burn_redeem_token(&payment_amount); | ||
self.accepted_token_balance() | ||
.update(|balance| *balance -= &payment_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.
Verifiy that balance
is greater than payment_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.
There's really no need for that. This should never fail, and even if it does, the framework has checks for BigUint diff being lower than 0.
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.
Looks good.
require!(payment_token == &redeem_token_id, INVALID_PAYMENT_ERR_MSG); | ||
|
||
let launched_token_supply = self.launched_token_balance().get(); | ||
if launched_token_supply == 0 { |
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.
How could you get to this situation? What if launched_token_supply is > 0 but < than the SC balance?
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.
If the owner doesn't deposit any tokens. I'm not sure I get the second question? How would you ever reach such a situation?
-> SingleValueMapper<BigUint>; | ||
fn redeem_token_total_circulating_supply(&self) -> SingleValueMapper<BigUint>; | ||
|
||
// TODO: Save launched token balance somewhere |
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 this still relevant?
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.
Nope. Removed.
No description provided.