scaffold template and implement ERC20JDI contract #1
Conversation
|
@princearoragithub @0xChicken1 Do you have time to review this? |
There was a problem hiding this comment.
Overall, looks pretty good. Some comments on syntax and javadocs
Had a question for @princearoragithub, it seems like we want to use camel case for all ERC20 interfaces, but snake case for all other functions?
| _start_epoch_time::write( | ||
| (get_block_timestamp().into() + INFLATION_DELAY - RATE_REDUCTION_TIME).into() | ||
| ); | ||
| _mining_epoch::write(u256_from_felt252(0)); // different from curve |
There was a problem hiding this comment.
Why set it differently from Curve? Curve is -1 right?
There was a problem hiding this comment.
yes, since cairo1 currently does not support signed integer yet. I think we can start from 0.
| } | ||
|
|
||
| #[view] | ||
| fn total_supply() -> u256 { |
There was a problem hiding this comment.
For consistency with Jediswap ERC20 interface, we can use camel case
There was a problem hiding this comment.
actually I think we should use snake case in order to follow the standard.
There's a dissussion about this in starknet forum: https://community.starknet.io/t/the-great-interface-migration/92107
| } | ||
|
|
||
| #[view] | ||
| fn balance_of(account: ContractAddress) -> u256 { |
| } | ||
|
|
||
| #[external] | ||
| fn transfer_from(sender: ContractAddress, recipient: ContractAddress, amount: u256) -> bool { |
There was a problem hiding this comment.
| } | ||
|
|
||
| #[external] | ||
| fn increase_allowance(spender: ContractAddress, added_value: u256) -> bool { |
There was a problem hiding this comment.
| } | ||
|
|
||
| #[external] | ||
| fn decrease_allowance(spender: ContractAddress, subtracted_value: u256) -> bool { |
There was a problem hiding this comment.
|
|
||
| struct Storage { | ||
| // Special address | ||
| _minter: ContractAddress, |
There was a problem hiding this comment.
Could you add javadocs for each of the variables in the struct?
Similar to here: https://github.com/jediswaplabs/JediSwap/blob/31-upgrade-to-cairo-10/src/PairC1.cairo#L53-L64
There was a problem hiding this comment.
Thanks for review. I've added.
| const INFLATION_DELAY: felt252 = 86400; // one day | ||
|
|
||
| #[event] | ||
| fn UpdateMiningParameters(time: u64, rate: u256, supply: u256) {} |
There was a problem hiding this comment.
Add javadocs to events similar to here: https://github.com/jediswaplabs/JediSwap/blob/31-upgrade-to-cairo-10/src/PairC1.cairo#L66-L68
| _start_epoch_supply::write(initial_supply); | ||
| } | ||
|
|
||
| #[view] |
There was a problem hiding this comment.
Could you add javadocs to all the functions that don't have it to keep consistent with Jediswap's other repos?
There was a problem hiding this comment.
Thanks for review. I've added.
src/erc20_jedi.cairo
Outdated
| cur_epoch_time += RATE_REDUCTION_TIME.into(); | ||
| }; | ||
| return to_mint; | ||
|
|
There was a problem hiding this comment.
Thanks for review. I've fixed it.
No description provided.