Skip to content
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

CT Transfer types #1899

Draft
wants to merge 31 commits into
base: main
Choose a base branch
from
Draft

CT Transfer types #1899

wants to merge 31 commits into from

Conversation

dssei
Copy link
Contributor

@dssei dssei commented Oct 16, 2024

Describe your changes and provide context

This PR contains transfer types to support confidential transfers feature

Testing performed to validate your change

  • Unit testing

Copy link

codecov bot commented Oct 16, 2024

Codecov Report

Attention: Patch coverage is 62.76596% with 175 lines in your changes missing coverage. Please review.

Project coverage is 61.37%. Comparing base (dd940e5) to head (a9e6ad5).

Files with missing lines Patch % Lines
x/confidentialtransfers/types/zk.go 61.17% 88 Missing and 44 partials ⚠️
x/confidentialtransfers/types/msgs.go 63.04% 25 Missing and 9 partials ⚠️
x/confidentialtransfers/types/codec.go 53.84% 6 Missing ⚠️
x/confidentialtransfers/types/cryptography.go 88.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1899      +/-   ##
==========================================
+ Coverage   61.35%   61.37%   +0.01%     
==========================================
  Files         263      267       +4     
  Lines       23328    23798     +470     
==========================================
+ Hits        14314    14606     +292     
- Misses       8009     8133     +124     
- Partials     1005     1059      +54     
Files with missing lines Coverage Δ
x/confidentialtransfers/types/cryptography.go 88.00% <88.00%> (ø)
x/confidentialtransfers/types/codec.go 53.84% <53.84%> (ø)
x/confidentialtransfers/types/msgs.go 63.04% <63.04%> (ø)
x/confidentialtransfers/types/zk.go 61.17% <61.17%> (ø)

... and 2 files with indirect coverage changes

@dssei dssei changed the title Ciphertext dto for ct CT types Oct 17, 2024
@mj850
Copy link
Contributor

mj850 commented Oct 17, 2024

This LGTM so far, to summarize my understanding here:

  1. client creates 'usable' transfer object usableTransferObject *Transfer -> This is a version of the object where the ciphertexts/curves.Points are in their usable form.
  2. client creates the MsgTransfer object by converting usableTransferObject (maybe by calling some function like MsgTransfer.ToProto(usableTransferObject)) - we need to do this conversion because the *Transfer object is not proto marshallable
  3. This message is marshalled as a proto and sent to the server side
  4. Server side unmarshals the message and it arrives at the handler as a MsgTransfer object.
  5. Server converts the MsgTransfer back to a usable transfer object *Transfer by calling MsgTransfer.FromProto()

Does this sound right?

Questions:

  1. We need to perform the intermediate steps 2 and 4 because the regular Transfer object (or more specifically the curves.Points and curves.Scalar objects) are not proto marshallable?
  2. Since we forked the kryptology library, are there any changes we can make there to enable us to marshal the *Transfer object directly without conversion to MsgTransfer?
  3. Should we have some method 'ToProto' that converts a Transfer object to a MsgTransfer?
  4. We would likely have to expose the types somehow since we would want external users other than the seid cli to be able to create valid MsgTransfer objects as well. We could add these type in here for now, but it might make sense to think about whether we can eventually make these types and msg generating functions external

)

func TestMsgTransfer_FromProto(t *testing.T) {
testDenon := "factory/sei1ft98au55a24vnu9tvd92cz09pzcfqkm5vlx99w/TEST"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testDenom*


remainingBalance := uint64(200)

// Encrypt the amount using source and destination public keys
Copy link
Contributor

@mj850 mj850 Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to make this its own function?

func ToProto(transfer *Transfer) *MsgTransfer {}

@dssei dssei changed the title CT types CT Transfer types Oct 31, 2024
mj850 and others added 6 commits November 1, 2024 15:16
* scaffolding for new module

* keeper scaffolding

* scaffold with types

* go sum

* comments pt1

* comments

* drop prev580
* pending balance type

* convert apply pending balance to a message type

* close account type
* 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
* Genesis draft

* revert evm params

* revert evm params -2

* working simple test

* linting

* addressing linter errors

* remove creating address for now
* add genesis init/export tests

* refactor store

* confidential transfers queries

* working draft test

* more tests

* refactor tests

* all accounts query

* formatting

* add pagination to request/response

* update implementation to use paginated response instead

* formatting

* remove redundant param

* all accounts with denoms

* clean up commented code

* return GetAccount back to keeper

* formatting
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants