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

Initial code + basic workflows #1

Merged
merged 15 commits into from
Oct 16, 2024
Merged

Initial code + basic workflows #1

merged 15 commits into from
Oct 16, 2024

Conversation

dssei
Copy link
Collaborator

@dssei dssei commented Oct 7, 2024

This PR adds:

  • the first part of the cryptography library - encryption.
  • essential github workflows
    • unit test
    • linting
    • test coverge

Testing

  • unit tests

@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@dssei dssei requested a review from mj850 October 8, 2024 23:34
@dssei dssei self-assigned this Oct 8, 2024
Copy link
Contributor

@mj850 mj850 left a comment

Choose a reason for hiding this comment

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

Thanks for making these changes! Love the new tests, workflow and cleaner code.

We should discuss the interface of the teg object: I see the utility of being able to swap out the curves in the future for other uses, but also think Static functions would be slightly easier to use and understand.

If we want this to be a general use encryption library, maybe we should add an application specific layer that uses the ED25519 curve directly, and deals with application specific things like denoms etc.

maxMapping map[MaxBits]bool
}

func NewTwistedElgamal(curve *curves.Curve) *TwistedElGamal {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should abstract away the curve from the user here - once we commit to a curve all ciphertexts/points are going to be on that curve so it's unlikely that we'll ever change the curve used. Asking user to input the curve here would add an extra step of having to find out what curve our application is using.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, got your point. Wanted to keep it more generic, with ability to plug in other curves if we decide so. I can add specific constructor for ed25519 curve.

}

func TestTwistedElGamal_DecryptWithZeroBits(t *testing.T) {
denom := "factory/sei1239081236472sd/testToken"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Maybe we can add this as CONST at the top of the file

)

// H_STRING H is a random point on the elliptic curve that is unrelated to G.
const H_STRING = "gPt25pi0eDphSiXWu0BIeIvyVATCtwhslTqfqvNhW2c"
Copy link
Contributor

Choose a reason for hiding this comment

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

Haven't tested but this H value might be specific to the ED25519 curve

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, will add the test with another curve to see if this still works.

}

// Initialize the scalar (private key) using the generated bytes
s, err := curves.ED25519().Scalar.SetBytesWide(scalarBytes[:])
Copy link
Contributor

Choose a reason for hiding this comment

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

This function uses the ED25519 curve directly - should either do this throughout (my preference) or use the curve under the teg object.

G := teg.GetG()
// Create a scalar from the amount.
bigIntAmount := new(big.Int).SetUint64(amount)
scalarAmount, err := curves.ED25519().Scalar.SetBigInt(bigIntAmount)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment here with using the curve

G := teg.GetG()
// Create a scalar from the amount.
bigIntAmount := new(big.Int).SetUint64(amount)
scalarAmount, err := curves.ED25519().Scalar.SetBigInt(bigIntAmount)
Copy link
Contributor

Choose a reason for hiding this comment

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

Using the curve directly here

- Plug in new ED25519 curve constructors in tests
@dssei
Copy link
Collaborator Author

dssei commented Oct 9, 2024

@mj850 addressed the comments, ptal

maxMapping map[MaxBits]bool
}

func NewTwistedElgamalWithED25519Curve() *TwistedElGamal {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we call this NewDefaultTwistedElgamal instead?

I imagine the users of this library would probably either one of:
us - for building the module in chain - would only use ED25519

  • not important since to us since we know what curve our module is using

sei builders - who want to build dapps to interact with the confidential tokens module etc.

  • Confusing because they need to figure out what curve to use?

other builders - who just want some encryption library

  • not important since they can use whatever curve they want

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed the other constructor, making ED25519 curve the only option, and caching it in the field

}

// EncryptWithRand encrypts a message using the public key pk and a given random factor.
func (teg TwistedElGamal) EncryptWithRand(pk curves.Point, message uint64, randomFactor curves.Scalar) (*Ciphertext, curves.Scalar, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually would recommend not exposing this function - it was added to test certain properties about the proofs, but this should never be used to encrypt stuff since encryption should be done with a random value to be secure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Made it private for now.


// Convert the byte arrays back into curve points
// Assuming `FromCompressed` is a method to parse compressed points
pointC, err := curves.ED25519().Point.FromAffineCompressed(temp.C)
Copy link
Contributor

Choose a reason for hiding this comment

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

Another instance of assuming the curve.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will keep it (since ED25519) would be the only option, but reduce to just one invocation.

@dssei
Copy link
Collaborator Author

dssei commented Oct 9, 2024

@mj850 addressed comments, ptal

Copy link
Contributor

@mj850 mj850 left a comment

Choose a reason for hiding this comment

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

LGTM!

@dssei dssei merged commit b5ce267 into main Oct 16, 2024
11 checks passed
@dssei dssei deleted the feature/init_and_workflows branch October 16, 2024 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants