Conversation
Signed-off-by: Jakub Sztandera <oss@kubuxu.com>
Signed-off-by: Jakub Sztandera <oss@kubuxu.com>
Signed-off-by: Jakub Sztandera <oss@kubuxu.com>
Signed-off-by: Jakub Sztandera <oss@kubuxu.com>
Co-authored-by: Jakub Sztandera <oss@kubuxu.com>
Co-authored-by: Rod Vagg <rod@vagg.org>
Co-authored-by: Rod Vagg <rod@vagg.org>
This reverts commit c76237c.
rvagg
left a comment
rvagg
left a comment
There was a problem hiding this comment.
This all seems good to me; I'd like to have @hugomrdias's eyes on it though before merging, particularly for the synapse-core stuff.
There's also the comment about covering the fallback case in tests I wouldn't mind in here too.
| }, | ||
| }, | ||
| { | ||
| name: 'ProviderIdSet', |
There was a problem hiding this comment.
| name: 'ProviderIdSet', | |
| name: 'Endorsements', |
its called endorsements in other places
There was a problem hiding this comment.
The contract is called ProviderIdSet. Endorsements is-a ProviderIdSet.
There was a problem hiding this comment.
sure either way call it one of those everywhere.
address constant is called ENDORSEMENTS_ADDRESS_MAINNET
i prefer endorsements but im fine with whatever you think is best
There was a problem hiding this comment.
All of our other contracts were singletons but this one is not. We could have numerous ProviderIdSet for purposes besides endorsements; for example we could also use them for the approved SP list and deprecate the list currently living in FWSS. In the opposite direction, they can also be used for blacklisting or sanctions compliance.
github-project-automation
bot
moved this from 🔎 Awaiting review
to ⌨️ In Progress
in FOC
hugomrdias
left a comment
hugomrdias
left a comment
There was a problem hiding this comment.
LGTM, just pending the naming thing and the awaits
| }, | ||
| }, | ||
| { | ||
| name: 'ProviderIdSet', |
There was a problem hiding this comment.
sure either way call it one of those everywhere.
address constant is called ENDORSEMENTS_ADDRESS_MAINNET
i prefer endorsements but im fine with whatever you think is best
|
The build failure is caused by merging master (which github does for approved PRs). I pulled the latest ABI changes in this PR, which is incompatible with #550 |
5 participants
Reviewer @rvagg @hugomrdias
Closes #540
Replaces #443
Changes
TODO multisig support for endorsement