api
api copied to clipboard
polkadot-js
Fix multi call of defenderVotes in the society module of api-derive
Thanks for reviewing @jacogr.
I wanted to add a regression test too, I even committed a first version, but I removed it because I couldn't figure out how to mock the society.members() response from the RPC. Without a mocked response the members array is empty and the error on defenderVotes doesn't get triggered.
Let me know if there's an easy way to mock this response and I can add the test back. If you don't mind the absence of test, you can go ahead and merge, I tested manually and seems to be working just fine.
By the way, I just found another smaller issue and committed a change...
I noticed that isDefenderVoter is only true if the member voted for the approval of the Defender.
In my opinion we should return true if the member voted, doesn't matter if it's an approval or a rejection, just like it was before with _membersPrev
By the way, I just found another smaller issue and committed a change...
I noticed that
isDefenderVoteris only true if the member voted for the approval of the Defender. In my opinion we should return true if the member voted, doesn't matter if it's an approval or a rejection, just like it was before with_membersPrev
I agree that the change here makes sense. Nice catch, I am not sure what the original purpose was around having the isDefenderVoter point to whether the vote was an approval for _membersCurr, but it doesn't seem to be in line with what _membersPrev is or the naming of the key so nice catch .
@laurogripa, due to some circumstances can you create a new PR with a description of the changes, and I'll give it a check and we can merge it in?
@laurogripa, due to some circumstances can you create a new PR with a description of the changes, and I'll give it a check and we can merge it in?
Hey, @TarikGul, thanks for reviewing. Does it need to be a new PR, or just an update of the description is enough?
@laurogripa, due to some circumstances can you create a new PR with a description of the changes, and I'll give it a check and we can merge it in?
Hey, @TarikGul, thanks for reviewing. Does it need to be a new PR, or just an update of the description is enough?
A new PR would be amazing, I know it's not ideal but there is no way around the Blocked Merging currently.
Yes, sorry for the delay, haven’t had a lot of time lately, but I’ll arrange to port to a new PR in the next few days
On Wed, 20 Mar 2024 at 10:24 Tarik Gul @.***> wrote:
@laurogripa https://github.com/laurogripa Still available to port this to a new PR?
— Reply to this email directly, view it on GitHub https://github.com/polkadot-js/api/pull/5767#issuecomment-2009558446, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABO6VNDR5Z6FG24UAJE4V2LYZGEY5AVCNFSM6AAAAABAGQQAYGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMBZGU2TQNBUGY . You are receiving this because you were mentioned.Message ID: @.***>
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
