update to new pattern in cairo2 #5
Conversation
| let owner = erc721_self.owner_of(:token_id); | ||
| let master = self._master.read(); | ||
| let masterDispatcher = IMasterDispatcher { contract_address: master }; | ||
| // @notice this is a sample SBT contract for dev guild, update the next line before deploying other guild |
There was a problem hiding this comment.
Since it's only for a dev guild contract what do you think about changing the name also? Like DevGuildSBT
There was a problem hiding this comment.
Its just a temporary name
| assert (tier != 0, 'NOT_ENOUGH_POINTS'); | ||
| let token_id = self._next_token_id.read(); | ||
| erc721_self._mint(to: account, token_id: token_id.into()); | ||
| self._wallet_of_owner.write(account, token_id); |
There was a problem hiding this comment.
Have you considered adding events on _wallet_of_owner and _token_type write?
There was a problem hiding this comment.
I don't think its necessary
There was a problem hiding this comment.
as transfer/mint event take care of _wallet_of_owner update and don't think it is necessary to index _token_type
| @@ -0,0 +1,97 @@ | |||
| use array::{Array, ArrayTrait, SpanTrait}; | |||
There was a problem hiding this comment.
Test a case when a contribution_levels arr = [100] and a user collected 100 dev points. The tier should be 1 in this situation. I guess it returns 0 right now
There was a problem hiding this comment.
100 dev points return tier 1 only.
| assert (tier1 == 1, 'invalid tier1'); | ||
|
|
||
| let tier2 = guildSBT_dispatcher.get_contribution_tier(user2()); | ||
| assert (tier2 == 2, 'invalid tier2'); |
There was a problem hiding this comment.
I'd suggest testing all four tiers for all guilds (since the logic for each of them is not in the same place like in a general function)
There was a problem hiding this comment.
have generalised the function
|
The code is pretty good. Two little suggestions.
|
| fn get_problem_solving_points(self: @ContractState, contributor: ContractAddress) -> u32 { | ||
| let contribution = self._contributions.read(contributor); | ||
| contribution.problem_solving | ||
| fn get_guild_total_contribution(self: @ContractState, month_id: u32, guild: felt252) -> u32 { |
There was a problem hiding this comment.
Question - how detailed should tests be? If very then we might want to test get_guild_total_contribution and get_guild_points functions for every guild in a specific test.
| @@ -390,7 +405,7 @@ mod Master { | |||
| if(new_contribution_score != 0) { | |||
| let mut contribution_data = self._contributions_data.read((contributor, guild)); | |||
| contribution_data.append(month_id); | |||
| contribution_data.append(new_guild_score); | |||
| contribution_data.append(new_contribution_score); | |||
There was a problem hiding this comment.
Could you create a test for this case? We missed it
| @@ -390,7 +405,7 @@ mod Master { | |||
| if(new_contribution_score != 0) { | |||
| let mut contribution_data = self._contributions_data.read((contributor, guild)); | |||
| contribution_data.append(month_id); | |||
There was a problem hiding this comment.
Indents are not needed. Should be inline with let mut contribution_data
No description provided.