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

feat: add claim orders #3

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

feat: add claim orders #3

wants to merge 10 commits into from

Conversation

vasyafromrussia
Copy link
Collaborator

Change claim process to give a grant issuer priority right to buy tokens.

Tokens grant claim

Copy link
Collaborator

@VladasZ VladasZ left a comment

Choose a reason for hiding this comment

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

Some small things, otherwise LGTM

self.orders.insert(&account_id, &vec![]);
}

let mut account_orders = self.orders.get(&account_id).unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Expect with error message. Or like we do it in jars:
.unwrap_or_else(|| env::panic_str(&format!("Jar with id: '{}' doesn't exist", id.0)))

for account_id in account_ids {
let mut order_execution = OrderExecution::new(account_id.clone());

let account_orders = self.orders.get(&account_id).expect("Account not found");
Copy link
Collaborator

Choose a reason for hiding this comment

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

[nit]
Add to error message which account was not found.

let account_orders = self.orders.get(&account_id).expect("Account not found");
for order in account_orders {
let requested_amount = order.claim_amount.0;
let approved_amount = (requested_amount as f32 * percentage) as u128;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it ok that percentage here is f32? Shouldn't it be Decimal like in jars?

}

self.internal_save_account_lockups(&order.account_id, account_lockup_indices);
// endregion
Copy link
Collaborator

@VladasZ VladasZ Jul 23, 2024

Choose a reason for hiding this comment

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

What is this?

// region cleanup
// endregion

pub struct OrderExecution {
pub account_id: AccountId,
pub total_approved: Balance,
pub details: HashMap<LockupIndex, (Balance, Balance)>, // (Balance approved, Balance refund)
Copy link
Collaborator

Choose a reason for hiding this comment

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

[nit]
Create struct so this comment isn't needed?

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.

2 participants