Add msg_server methods #1923
Add msg_server methods #1923
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // Store the account | ||
| m.Keeper.SetAccount(ctx, address, req.Denom, account) | ||
|
|
||
| ctx.EventManager().EmitEvents(sdk.Events{ |
There was a problem hiding this comment.
Seems like EmitTypedEvents can be used instead, since EmitEvents is deprecated
There was a problem hiding this comment.
Going to add a todo to look into this a bit later
| sdk.NewEvent( | ||
| types.EventTypeInitializeAccount, | ||
| sdk.NewAttribute(types.AttributeDenom, instruction.Denom), | ||
| sdk.NewAttribute(types.AttributeAddress, instruction.FromAddress), |
There was a problem hiding this comment.
There probably could be several events like this for single account, which may look all the same for indexer. Adding public key might help to distinguish between them
There was a problem hiding this comment.
Hmm this particular event is specific to the denom and address so I think we should be good! Pubkey generation is deterministic actually so even if the user closes and reinitializes their account here, it should be the same pubkey
There was a problem hiding this comment.
Never mind, I think denom makes it unique
| } | ||
|
|
||
| // Check if the account exists | ||
| address, err := sdk.AccAddressFromBech32(req.FromAddress) |
There was a problem hiding this comment.
Looks like checking address and returning account is logic that is repeated (and could be extracted into seperate method and re-used)
There was a problem hiding this comment.
Refactored the keeper methods (get, set and delete accounts) to take in address as a string instead and try to perform this conversion, since the only place that needs this sdkAccAddress is the keeper for now.
| // Convert the instruction to proto. This also validates the request. | ||
| instruction, err := req.FromProto() | ||
| if err != nil { | ||
| return nil, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "Invalid Msg") |
There was a problem hiding this comment.
nit: general recommendation is to not capitalize error messages https://go.dev/wiki/CodeReviewComments#error-strings
There was a problem hiding this comment.
Makes sense, uncapitalized all the error messages in msg, zk and msg_server
|
|
||
| // Split the deposit amount into lo and hi bits. | ||
| // Extract the bottom 16 bits (rightmost 16 bits) | ||
| bottom16 := uint16(req.Amount & 0xFFFF) |
There was a problem hiding this comment.
I think we could follow the PoC approach and have a split logic in a separate method. May simplify testing of it as well
There was a problem hiding this comment.
Added a utils package in the module for stuff like this. There's probably 1 or 2 other commonly used things I can add there when I think of them
| return nil, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "Invalid Msg") | ||
| } | ||
|
|
||
| // Verify that the account has sufficient funds (Remaining balance after making the transfer is greater than or equal to zero.) |
There was a problem hiding this comment.
nit: let's try to keep the line size within 120 chars
|
|
||
| // Return the tokens to the sender | ||
| coins := sdk.NewCoins(sdk.NewCoin(instruction.Denom, sdk.NewIntFromUint64(instruction.Amount))) | ||
| if err := m.Keeper.SendTokens(ctx, address, coins); err != nil { |
There was a problem hiding this comment.
Out of scope for this method, but curious, how do we prevent funds in module address to not be sent anywhere, say other module address.
There was a problem hiding this comment.
Hmmm haven't thought about that. Is this an issue that we need to be aware of?
| // Get the user's account | ||
| account, exists := m.Keeper.GetAccount(ctx, address, req.Denom) | ||
| if !exists { | ||
| return nil, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "Account does not exist") |
There was a problem hiding this comment.
There is also not found sdk error
ErrNotFound = Register(RootCodespace, 38, "not found")
There was a problem hiding this comment.
Makes sense! Refactored all the account not found errors to ErrNotFound
| return nil, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "Error summing balances") | ||
| } | ||
|
|
||
| zeroCiphertextLo, err := elgamal.SubtractCiphertext(account.PendingBalanceLo, account.PendingBalanceLo) |
There was a problem hiding this comment.
Curious, if we need to subtract balance from itself. Why not alternatively just encrypt 0? Can we store nil instead? Also curious if this can be a source of non-determinism
There was a problem hiding this comment.
We use subtraction here since Encryption is non deterministic (since it generates randomness), but subtraction is a deterministic operation.
From a confidentiality pov, It somewhat makes sense to store nil here since the pending balance counter will be set to 0 instead so everyone knows there are no pending balances in the account.
But from a implementation perspective I think its much easier to deal with accounts if we just assume the pending balances and available balances are always a ciphertext (don't have to check for nil, write special edge cases for when pending balance is nil etc, we can just perform arithmetic on the ciphertext)
* new types * add deposit to msgs * add import issues * revert evm/params to old state * withdraw with tests * small changes * refactor and add tests * comment * refactor initialize type * types * non module account types * msg server methods with module accounts * tests scaffold * add msg server tests * codeql * test issues * test improvements * comments * todo * app
Describe your changes and provide context
Adds server side logic to msg_server, as well as sets up the module account so this module can send and receive tokens.
Testing performed to validate your change
Writing msg_sever_tests right now.