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 types #1899

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

CT types #1899

wants to merge 8 commits into from

Conversation

dssei
Copy link
Contributor

@dssei dssei commented Oct 16, 2024

Describe your changes and provide context

Ciphertext dto for ct

Testing performed to validate your change

  • Unit testing

Copy link

codecov bot commented Oct 16, 2024

Codecov Report

Attention: Patch coverage is 64.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 61.21%. Comparing base (fc9a0bd) to head (331b7dd).

Files with missing lines Patch % Lines
x/confidentialtransfers/types/cryptography.go 64.00% 6 Missing and 3 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1899      +/-   ##
==========================================
- Coverage   61.37%   61.21%   -0.17%     
==========================================
  Files         263      264       +1     
  Lines       23314    23339      +25     
==========================================
- Hits        14310    14287      -23     
- Misses       7995     8042      +47     
- Partials     1009     1010       +1     
Files with missing lines Coverage Δ
x/confidentialtransfers/types/cryptography.go 64.00% <64.00%> (ø)

... 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 {}

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