-
Notifications
You must be signed in to change notification settings - Fork 267
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use FederationChangeFunctions enum when voting #2897
Use FederationChangeFunctions enum when voting #2897
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
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.
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.
Should we update unit tests?
default: | ||
// Fail by default | ||
logger.warn("[executeVoteFederationChangeFunction] Unrecognized called function."); | ||
result = new ABICallVoteResult(false, FederationChangeResponseCode.GENERIC_ERROR.getCode()); |
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.
this is no needed since the method receives the FederationChangeFunction
as an input
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.
// Try to do a dry-run and only register the vote if the | ||
// call would be successful |
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.
Not sure this comment belongs here, the dryRun is a parameter. It's on voteFederationChange
function where the dryRun is first attempted
} | ||
|
||
private ABICallVoteResult executeVoteFederationChangeFunction(boolean dryRun, ABICallSpec callSpec, BridgeEventLogger eventLogger) throws BridgeIllegalArgumentException { | ||
private ABICallVoteResult executeVoteFederationChangeFunction(boolean dryRun, byte[][] callSpecArguments, FederationChangeFunction federationChangeFunction, BridgeEventLogger eventLogger) throws BridgeIllegalArgumentException { |
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.
Not a fan of receiving a byte[][]
parameter. I think it let's you pass whatever. Wouldn't it be better to keep passing the ABICallSpec obejct
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.
yep, you right. I'll rollback the change
} | ||
|
||
return result; | ||
return new ABICallVoteResult(executionResult == 1, executionResult); |
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.
return new ABICallVoteResult(executionResult == 1, executionResult); | |
boolean executionWasSuccessful = executionResult == 1; | |
return new ABICallVoteResult(executionWasSuccessful, executionResult); |
BtcECKey btcPublicKey; | ||
ECKey rskPublicKey; | ||
ECKey mstPublicKey; | ||
try { | ||
btcPublicKey = BtcECKey.fromPublicOnly(callSpec.getArguments()[0]); | ||
btcPublicKey = BtcECKey.fromPublicOnly(callSpecArguments[0]); |
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.
Looks like callSpecArguments[] is read twice usually, inside the try and then inside the catch. Perhaps we should store in a variable to a avoid possible errors by mixing up the indexes?
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.
nice catch!
…able that can be reused. Modify method to not receive bytes as an argument
815d2c5
to
98a1d89
Compare
Quality Gate passedIssues Measures |
8588748
into
feature/powpeg_validation_protocol-phase4
No description provided.