-
Notifications
You must be signed in to change notification settings - Fork 662
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
P-chain: updating wallet to use dynamic fee calculator #3189
Conversation
6b8d844
to
07456ae
Compare
52ae6a1
to
048a14c
Compare
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.
overall looks good. few small comments.
@@ -252,7 +255,11 @@ func newEnvironment(t *testing.T, ctrl *gomock.Controller, f fork) *environment | |||
} | |||
|
|||
func addSubnet(env *environment) { | |||
builder, signer := env.factory.NewWallet(preFundedKeys[0]) | |||
builder, signer, feeCalc, err := env.factory.NewWallet(preFundedKeys[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.
maybe add the t *testing.T
to the addSubnet
instead of panic'ing ?
} | ||
|
||
// GetNextFeeRates returns the next fee rates that a transaction must pay to be accepted now | ||
func (s *Service) GetDynamicFeeConfig(_ *http.Request, _ *struct{}, reply *DynamicFeesConfigReply) error { |
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.
The method, as implemented, makes me feel a bit uncomfortable. The reply
is being used in some cases, but not in others, and the caller is expected to deduce that by examining the error code. In addition, the caller is expected to provide a valid reply
pointer, or the method might (sometimes) panic.
I know that this aligns with the existing calling convension in this file, and was wondering if there is a good reason for that. In particular, I would that written it as
func (s *Service) GetDynamicFeeConfig(_ *http.Request, _ *struct{}) (reply *DynamicFeesConfigReply, err error)
since that would have the convention that the err
and the reply
are mutually exclusive.
Why this should be merged
Splitting introduction of dynamic fees into 4 PRs:
How this works
How this was tested
Extra UTs. All new code is not wired in yet. Will be done in a sequent PR