Return isGlobalSkill boolean flag in ColonyNetwork.getSkill #279
Conversation
contracts/ColonyNetwork.sol
Outdated
| function getSkill(uint256 _skillId) public view returns (uint256, uint256) { | ||
| return (skills[_skillId].nParents, skills[_skillId].nChildren); | ||
| function getSkill(uint256 _skillId) public view returns (uint256, uint256, bool) { | ||
| Skill storage skill = skills[_skillId]; |
There was a problem hiding this comment.
You have probably missed my previous comment
I'd be interested if we can start returning structs out of functions. This should be possible with the ABIEncoder2 experimental feature.
Essentially I'm asking if we simply return the entire Skill struct here?
There was a problem hiding this comment.
@elenadimitrova Looks doable. Only thing is we would need to extract the Skill struct definition from ColonyNetworkStorage and place it in a library so iColonyNetwork.sol can access the Skill type for function return declaration. Thoughts?
There was a problem hiding this comment.
I think this might warrant a bigger refactoring piece with some upgrades testing. Might log it separately so we don't have to invent the design here. I like the library idea though. Will be the likely winner as to how we implement this but will need to consider upgrade options, e.g. what happens when a new struct member is added, can EtherRouter handle that without reregistering in the Resolver and upgrading the contract implementation, e.g. the ColonyNetwork in the above case.
There was a problem hiding this comment.
Ok sounds good to me. Moving this back to ready-for-review as I have addressed the other comment regarding moving the tests.
There was a problem hiding this comment.
Now logged separately in #283
test/colony-network.js
Outdated
| @@ -295,4 +295,22 @@ contract("ColonyNetwork", accounts => { | |||
| assert.equal(skillCount.toNumber(), 1); | |||
| }); | |||
| }); | |||
|
|
|||
| describe("when getting a skill", () => { | |||
There was a problem hiding this comment.
Can we move these tests to meta-colony.js? That's where all skills related functionality is held atm
KevinLiLu
force-pushed
the
feature/249-isGlobalSkill-flag-in-ColonyNetwork-getSkill
branch
from
25e7d95 to
fbfd8b9
Compare
|
I think you missed my comment above #279 (review) ? |
|
I will push the above change to your branch (hope you don't mind) |
elenadimitrova
force-pushed
the
feature/249-isGlobalSkill-flag-in-ColonyNetwork-getSkill
branch
from
15036fb to
39b697e
Compare
elenadimitrova
force-pushed
the
feature/249-isGlobalSkill-flag-in-ColonyNetwork-getSkill
branch
from
39b697e to
fdfc184
Compare
|
@elenadimitrova Oops, good catch! |
Closes #249
Ignore the atrocious branch name 😋
isGlobalSkillboolean flag as the 3rd return value ofColonyNetwork.getSkillso users do not need to make an additional call toisGlobalSkill