-
Notifications
You must be signed in to change notification settings - Fork 14
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
Fix: withdraw pending balance #64
Conversation
e931607
to
1f31b75
Compare
95c6c32
to
d2a86b0
Compare
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.
A few comments regarding data structures and one spelling mistake across the project TokendId
-> TokenId
@@ -77,7 +80,7 @@ pub struct ProxyState { | |||
/// store incoming messages against status locks | |||
pub incoming_messages: RefCell<HashMap<MessageHash, MessageStatus>>, | |||
/// user balances | |||
pub balances: RefCell<HashMap<Principal, HashMap<TokendId, Nat>>>, | |||
pub balances: RefCell<HashMap<Principal, Vec<(Principal, Nat)>>>, |
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.
pub balances: RefCell<HashMap<Principal, Vec<(Principal, Nat)>>>, | |
pub balances: RefCell<HashMap<EthereumAddr, Nat>>, |
Could we narrow it down to this?
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 we need caller
then ignore
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 need the caller
@@ -91,7 +94,7 @@ pub struct StableProxyState { | |||
/// store incoming messages against status locks | |||
pub incoming_messages: HashMap<MessageHash, MessageStatus>, | |||
/// user balances | |||
pub balances: HashMap<Principal, HashMap<Principal, Nat>>, | |||
pub balances: Option<HashMap<Principal, Vec<(Principal, Nat)>>>, |
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.
pub balances: Option<HashMap<Principal, Vec<(Principal, Nat)>>>, | |
pub balances: RefCell<HashMap<EthereumAddr, Nat>>, |
The same usage can be applied 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.
Also Is Optional
being committed or only required for upgrades?
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.
it is required for the upgrades
let mut binding = self.balances.borrow_mut(); | ||
let balance = binding.entry(caller).or_default(); | ||
match balance.into_iter().find(|tx| tx.0 == to) { | ||
Some(value) => value.1 += amount, | ||
None => { | ||
balance.push((to, 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.
using a hashmap
in the original structure could simplify this a lot
RefCell<HashMap<Principal, HashMap<EthereumAddr, Nat>>>
pub fn update_balance(&self, caller: Principal, to: Principal, amount: Nat) { | ||
let mut binding = self.balances.borrow_mut(); | ||
let user_balances = binding.get_mut(&caller).unwrap(); | ||
// if new amount is zero, we remove the tx. | ||
if amount == 0 { | ||
let index = user_balances.into_iter().position(|tx| tx.0 == to).unwrap(); | ||
user_balances.remove(index); | ||
} else { | ||
let mut balance = user_balances.iter_mut().find(|tx| tx.0 == to).unwrap(); | ||
balance.1 = 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.
same as above, simplifying our structure
@@ -78,7 +81,7 @@ pub struct ProxyState { | |||
/// store incoming messages against status locks | |||
pub incoming_messages: RefCell<HashMap<MessageHash, MessageStatus>>, | |||
/// user balances | |||
pub balances: RefCell<HashMap<Principal, HashMap<TokendId, Nat>>>, | |||
pub balances: RefCell<HashMap<Principal, HashMap<TokendId, Vec<(Principal, Nat)>>>>, |
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.
Same idea as the eth_proxy
balances structure.
- Simplify
- and if caller is not needed truncate to:
RefCell<HashMap<EthereumAddr, HashMap<TokenId, , Nat>>>,
Some(balance) => balance.clone(), | ||
None => Nat::from(0_u32), | ||
}) | ||
pub fn get_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.
this will be simplified, if the structure is also simplified
be0acdc
to
748fc0a
Compare
4b95f1c
to
44d66f9
Compare
69c0b08
to
765207d
Compare
Description
Changes
withdraw(eth_contract_as_principal: TokendId, eth_addr: EthereumAddr, _amount: Nat,)
pub struct WithdrawableBalance(pub Vec<(String, String, Nat)>)