-
Notifications
You must be signed in to change notification settings - Fork 30
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
refactor(coinswap): returns the result value of swap #379
Conversation
…r/refactor-coinswap
WalkthroughThe changes revolve around enhancing the Changes
Poem
TipsChat with CodeRabbit Bot (
|
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- modules/coinswap/types/tx.pb.go
Files selected for processing (7)
- api/irismod/coinswap/tx.pulsar.go (15 hunks)
- modules/coinswap/keeper/keeper.go (5 hunks)
- modules/coinswap/keeper/msg_server.go (1 hunks)
- modules/coinswap/keeper/swap.go (2 hunks)
- modules/coinswap/keeper/swap_test.go (10 hunks)
- modules/coinswap/types/utils.go (1 hunks)
- proto/irismod/coinswap/tx.proto (1 hunks)
Files not summarized due to errors (1)
- api/irismod/coinswap/tx.pulsar.go: Error: Message exceeds token limit
Files skipped from review due to trivial changes (1)
- modules/coinswap/keeper/swap_test.go
Additional comments: 25
modules/coinswap/keeper/msg_server.go (1)
- 129-138: The changes in this hunk are well implemented. The
SwapCoin
function now returns anAmount
field in theMsgSwapCoinResponse
response type. TheSwap
method in them.k
object is now called with themsg
argument, and the returnedamount
and any error are handled accordingly. This change is consistent with the pull request summary and seems to be a good improvement to the function's design.proto/irismod/coinswap/tx.proto (1)
- 140-145: The new
amount
field inMsgSwapCoinResponse
is non-nullable, which means it must always contain a validcosmos.base.v1beta1.Coin
value. Ensure that all functions that return aMsgSwapCoinResponse
are updated to include this field and that they never return a null value foramount
.message MsgSwapCoinResponse { + cosmos.base.v1beta1.Coin amount = 1 [ (gogoproto.nullable) = false ]; }
modules/coinswap/keeper/swap.go (4)
69-97: The function
TradeExactInputForOutput
has been updated to returnsdk.Coin
instead ofsdkmath.Int
. This change is consistent and makes sense as the function is dealing with tokens (coins) and not just integers. The error handling and the logic of the function seem to be correct.108-144: The function
doubleTradeExactInputForOutput
has also been updated to returnsdk.Coin
instead ofsdkmath.Int
. The logic and error handling in this function also seem to be correct.192-217: The function
TradeInputForExactOutput
has been updated to returnsdk.Coin
instead ofsdkmath.Int
. The logic and error handling in this function seem to be correct.228-264: The function
doubleTradeInputForExactOutput
has been updated to returnsdk.Coin
instead ofsdkmath.Int
. The logic and error handling in this function seem to be correct.modules/coinswap/types/utils.go (4)
13-18: The new trade types
BuyDouble
andSellDouble
have been added. Ensure that these are handled correctly in all parts of the codebase where trade types are used.20-21: The new types
TradeType
andTradeFunc
have been introduced. Ensure that these are used correctly throughout the codebase.33-45: The new function
GetTradeType
has been added. It seems to correctly determine the trade type based on the given parameters. However, it would be good to add unit tests to verify its correctness.47-49: No changes have been made to the
GetReservePoolAddr
function.modules/coinswap/keeper/keeper.go (3)
25-29: The
tradeFuncs
map is a good addition to theKeeper
struct. It allows for easy extension of trade types and their corresponding functions in the future.54-66: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [48-62]
The
NewKeeper
function is correctly updated to initialize thetradeFuncs
map with the appropriate trade functions.
- 69-83: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [71-97]
The
Swap
function has been correctly updated to use thetradeFuncs
map to determine the trade function based on the trade type. The return type of theSwap
function has been changed to(sdk.Coin, error)
to return the swapped amount. However, it's important to ensure that all calls to this function throughout the codebase have been updated to handle the new return type.api/irismod/coinswap/tx.pulsar.go (12)
5194-5201: The initialization of the
MsgSwapCoinResponse
message descriptor and theamount
field descriptor looks correct. Ensure that theFile_irismod_coinswap_tx_proto.Messages().ByName("MsgSwapCoinResponse")
andmd_MsgSwapCoinResponse.Fields().ByName("amount")
calls return the expected descriptors.5269-5274: The
Range
function correctly checks if theAmount
field is notnil
before calling the provided functionf
. This prevents a potentialnil
pointer dereference.5290-5291: The
Has
function correctly checks if theAmount
field is notnil
before returningtrue
. This ensures that the function correctly reports whether theAmount
field is populated.5308-5309: The
Clear
function correctly sets theAmount
field tonil
. This ensures that the function correctly clears theAmount
field.5326-5328: The
Get
function correctly returns theAmount
field. This ensures that the function correctly retrieves theAmount
field.5349-5350: The
Set
function correctly sets theAmount
field to the provided value. This ensures that the function correctly sets theAmount
field.5371-5375: The
Mutable
function correctly checks if theAmount
field isnil
before creating a newv1beta1.Coin
and assigning it to theAmount
field. This ensures that the function correctly returns a mutable reference to theAmount
field.5389-5391: The
NewField
function correctly creates a newv1beta1.Coin
and returns it. This ensures that the function correctly creates a newAmount
field.5461-5464: The
Size
function correctly checks if theAmount
field is notnil
before calculating its size. This ensures that the function correctly calculates the size of theMsgSwapCoinResponse
message.5494-5507: The
Marshal
function correctly checks if theAmount
field is notnil
before marshaling it. This ensures that the function correctly marshals theMsgSwapCoinResponse
message.5557-5592: The
Unmarshal
function correctly checks the wire type and field number before unmarshaling theAmount
field. This ensures that the function correctly unmarshals theMsgSwapCoinResponse
message.6981:
TheMsgSwapCoinResponse
message correctly includes anAmount
field of type*v1beta1.Coin
. This ensures that the message correctly represents the response of theSwapCoin
function.
- 7004-7008: The
GetAmount
function correctly checks if theMsgSwapCoinResponse
message is notnil
before returning theAmount
field. This ensures that the function correctly retrieves theAmount
field.
returns the result value of swap
Summary by CodeRabbit
BuyDouble
andSellDouble
.Swap
function to handle errors more effectively.Coin
object instead of anInt
object.Swap
function to capture and handle errors.