-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
use warpRequirePrimaryNetworkSigners config #537
Conversation
d5ee2ba
to
45889ae
Compare
relayer/application_relayer.go
Outdated
// this shouldn't be reachable since if we found a quorum, we should also find the requirePrimaryNetworkSigners | ||
// but leaving the check for completeness |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's probably an indication we should be able to fetch them from the config together?
I also agree with your comment that the current WarpQuorum
object is unnecessary. We can just use a uint for QuorumPercentage
, or have WarpQuorum
contain the percentage and requirePrimaryNetworkSigners
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with the latter route and renamed it to WarpConfig
. It's basically identical to the source WarpConfig
defined in subnet-evm
but doesn't implement the Upgrade
interface and when converting it we populate it with warpDefaultQuorumNumerator
value if it's 0
relayer/config/config.go
Outdated
return s.warpRequirePrimaryNetworkSigners, nil | ||
} | ||
} | ||
return false, errFailedToGetWarpQuorum |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this error should say anything about the Warp Quorum. We error here iff the destination blockchain is in the config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the wording to indicate that the blockchainID is not in the destination blockchains. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, once @geoff-vball 's comments are addressed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
Why this should be merged
Fixes #454
How this works
Fetches the additional value from the config and uses it when creating application relayer struct.
Open q:
WarpQuorum
struct doesn't seem very useful since denominator is hardcoded at compile time to 100How this was tested
How is this documented