🐗 Boar Adventure - Review

[Majorfi]

Review and questions for the Boar Adventure

Scope :

• boarAdventure.sol
• loot.sol
• onlyExtended.sol
• rERC20.sol

[Majorfi]

OnlyExtended.sol

I did a PR prior to this thread to be able to change the Extended address. In some case it can be easier to deploy the contract with a private address, then attach the Extended role to the actual multisig. I added a 2 steps process (set and accept) similar to what Yearn uses in the Vaults contract.

[Majorfi]

rERC20.sol

My main concern come from the allowance. I would rather use something like this

-    mapping(uint => mapping (uint => uint)) public allowance;
+    mapping(address => mapping (uint => mapping (uint => uint))) public allowance;

English translation : An Address (owner of the Adventurer) can allow the spending for an Adventurer (uint) by another Adventurer (uint) a maximum amount of ERC20 (uint).
This was we prevent a vulnerability where you approve you adventurer, sell them, then stole all their item as they are already approved.
This pattern is used on the rarity_extended_xp contract

The _isApprovedOrOwner function does not use isApprovedForAll. Should we add that ?

[TheAustrian1998]

This was we prevent a vulnerability where you approve you adventurer, sell them, then stole all their item as they are already approved.

Good finding, we need to add that fix.

The _isApprovedOrOwner function does not use isApprovedForAll. Should we add that ?

I don't understand this part. I think isApprovedForAll function is a specific function from ERC721 Standard.

[TheAustrian1998]

Do you mean checking if it's approved for all in _isApprovedOrOwner?

[Majorfi]

Yep !

[Majorfi]

loot.sol

LGTM

[Majorfi]

BoarAdventure.sol

I did a pr with some changes, but here is more of them :)

It may be good to update the stats of the Dungeons. After some simulation here is my suggestion :

-    int public constant dungeon_health = 16;
-    int public constant dungeon_damage = 4;
-    int public constant dungeon_to_hit = 5;
-    int public constant dungeon_armor_class = 4;
+    int public constant dungeon_health = 16;
+    int public constant dungeon_damage = 7;
+    int public constant dungeon_to_hit = 5;
+    int public constant dungeon_armor_class = 5;

This way, a max strenght new adventurer won't survive the fight, but a more balanced one will.

• My warrior lvl 3 with 16 - 16 - 16 (Str, Dext, Const) will barely win the fight
• My barbarian lvl 3 with 20 - 8 - 15 will win the fight
• My warrior lvl 1 with 22 - 8 - 8 will loose the fight (twoshots)
• My barbarian lvl 1 with 22 - 8 - 8 will loose the fight (twoshots)
• My monk lvl 1 with 22 - 8 - 8 will loose the fight (oneshot)
• My warrior lvl 1 with 20 - 8 - 15 will loose the fight (twoshots)
  To ease things, damange could also be decrease to 6

---

For the _isApprovedOrOwner function, same comment as before, check also isApprovedForAll ?

---

For the fight function.
OK

---

For the reproduce function.

• It should require boar_population >= 2 imo
• Expected reward_type should be random too imo
• I have a doubt about reward = multiplier_points_by_level(reward, _level);. I wonder if this is not to much. I mean you are lvl 3, you can earn 3x the amount, it looks a bit overpowered imo. It's not used in the fight an I am not sure this should be used here neither

[TheAustrian1998]

It may be good to update the stats of the Dungeons. After some simulation here is my suggestion

Okay, I will add it, but keep in mind that your reward is based in points left of the battle. I mean, if you win the battle by a little, your reward is little too, example: the boar left your adventurer with 1HP. You win, but your reward will be 1 (and later boosted o decreased).

It should require boar_population >= 2 imo

True

Expected reward_type should be random too imo

Great!

I have a doubt about reward = multiplier_points_by_level(reward, _level);.

Yeah, do you suggest delete that multiplier?

[Majorfi]

Hum, you may be right for the first point. Maybe just decrease damage to 6 ?

Yep, or at least cap the amount of reward ?

[TheAustrian1998]

Yeah, that would be good. What about adding a function to change all of that parameters?