-
Notifications
You must be signed in to change notification settings - Fork 105
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
fix(axelarnet): remove unused TxID parameter from ConfirmDepositRequest #1218
Conversation
x/axelarnet/client/cli/tx.go
Outdated
burnerAddr, err := sdk.AccAddressFromBech32(args[2]) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
msg := types.NewConfirmDepositRequest(cliCtx.GetFromAddress(), txID, args[1], burnerAddr) | ||
msg := types.NewConfirmDepositRequest(cliCtx.GetFromAddress(), nil, args[1], burnerAddr) |
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.
update the contructor so not to ask for this parameter, but do not remove the field from the protobuf
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.
also, i almost forgot: mark the field as deprecated on the protobuf like this: https://github.com/axelarnetwork/axelar-core/blob/main/proto/axelarnet/v1beta1/tx.proto#L32
774a6be
to
931a560
Compare
proto/axelarnet/v1beta1/tx.proto
Outdated
bytes tx_id = 2 [ | ||
(gogoproto.customname) = "TxID", | ||
deprecated = true | ||
]; |
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.
bytes tx_id = 2 [ | |
(gogoproto.customname) = "TxID", | |
deprecated = true | |
]; | |
reserved 2; // tx_id was removed in v0.14 |
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 suggest just removing it and reserving the field number for backward compatibility.
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 "tx_id" also be reserved?
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.
Have reserved 2
at this stage
fix remove txID from constructor and update rest endpoint deprecate the txID field remove txID from ConfirmDeposit message and reserve its proto index
474bcfc
to
b112e66
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.
LGTM, but you are gonna have to see whats wrong with the auto generated files.
Description
TxID is not necessary because ConfirmDeposit confirms the full available balance rather than a specific transaction.
This PR removes the TxID parameter from the CLI command and the request message validation.
Todos
Steps to Test
Expected Behaviour
Other Notes