feat: verifySignature function #2776

Villaquiranm · 2024-09-09T20:09:16Z

related to #2777

Contributors' checklist...

Added new tests, or not needed, or not feasible
Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
Updated the official documentation or not needed
No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
Added references to related issues and PRs
Provided any useful hints for running manual tests
Added new benchmarks to generated graphs, if any. More info here.

codecov · 2024-09-09T20:13:19Z

Codecov Report

Attention: Patch coverage is 89.47368% with 4 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
gnovm/stdlibs/std/native.go	60.00%	2 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

notJoon · 2024-10-10T07:29:54Z

CI failed. could you please check and fix this?

…ture

Villaquiranm · 2024-10-11T17:20:10Z

CI failed. could you please check and fix this?

I think it is ok just one does not want to finish do not know why
@notJoon

…ture

notJoon

I checked it purely through test runs, it executed well without any significant issues.

Although I also looked into related issues, it might be better to conduct a more detailed review in the next stage.

remove: review/triage-pending

thehowl · 2024-10-23T14:32:11Z

I'm still not convinced by this approach. I think we should have verification happen at the protol level rather than happen at the Gno level. Can we not use the message signers as I pointed out in the discussion on #2777?

zivkovicmilos

I like the direction, but we need better naming and more tests 🙏

Thinking about this a bit, let's discuss if it's even required, and why

zivkovicmilos · 2025-01-08T08:19:32Z

gnovm/tests/files/std12_stdlibs.gno

@@ -0,0 +1,14 @@
+package main


Why not add multiple stdlib tests for valid and invalid signature verification?

zivkovicmilos · 2025-01-08T08:19:52Z

gnovm/stdlibs/std/native_test.go

+		t.Error("verify failed")
+	}
+
+	if signer != info.GetAddress().String() {


assert.Equal

zivkovicmilos · 2025-01-08T08:20:02Z

gnovm/stdlibs/std/native_test.go

+	}
+
+	signatureValid, _ = X_verifySignature(publicKey, maliciousMessage, signatureHex)
+	if signatureValid {


assert.True

zivkovicmilos · 2025-01-08T08:25:13Z

gnovm/stdlibs/std/native_test.go

+func TestVerify(t *testing.T) {
+	kb := keys.NewInMemory()
+	pass := "hardPass"
+	info, err := kb.CreateAccount("user", DefaultAccount_Seed, pass, pass, 0, 0)


You don't need to use the keybase to generate a key, you can just use something like ed25519.GenPrivKey() or even better secp256k1.GenPrivKey(), which will mimic what the keybase is doing under the hood

Not to mention it is exponentially faster 🚀

zivkovicmilos · 2025-01-08T08:25:58Z

gnovm/stdlibs/std/native_test.go

@@ -192,3 +196,31 @@ func TestPrevRealmIsOrigin(t *testing.T) {
 		})
 	}
 }
+
+func TestVerify(t *testing.T) {


Should be named TestVerifyECSignature or something similar

zivkovicmilos · 2025-01-08T08:31:06Z

gnovm/stdlibs/std/native.go

@@ -150,6 +152,22 @@ func X_encodeBech32(prefix string, bytes [20]byte) string {
 	return b32
 }

+func X_verifySignature(pubKeySigner string, msg string, signature string) (bool, string) {


Please, let's rename this method to something more descriptive, since this is the sig verification method for both ed25519 and secp256k1. What do you think about VerifyECSignature, as the name for the native method?

Also, I'd rename the args here:

pubKeySigner -> bech32PubKey (to be explicit)

msg -> rawData (and please make this a []byte)

change the type of signature to []byte

zivkovicmilos · 2025-01-08T08:31:22Z

gnovm/stdlibs/std/native.gno

@@ -56,6 +56,10 @@ func DecodeBech32(addr Address) (prefix string, bz [20]byte, ok bool) {
 	return decodeBech32(string(addr))
 }

+func VerifySignature(pubKeySigner string, signature string, msg string) (bool, string) {


Check my comment below for the rename 🙏

zivkovicmilos · 2025-01-08T08:31:48Z

gnovm/stdlibs/std/native_test.go

 )

+const DefaultAccount_Seed = "source bonus chronic canvas draft south burst lottery vacant surface solve popular case indicate oppose farm nothing bullet exhibit title speed wink action roast"


Please, drop this

zivkovicmilos · 2025-01-08T08:39:14Z

I'm still not convinced by this approach. I think we should have verification happen at the protol level rather than happen at the Gno level. Can we not use the message signers as I pointed out in the discussion on #2777?

@thehowl
I think what @Villaquiranm is aiming at is having generic signature verification, the signature doesn't have to originate from a message / transaction, but can be arbitrary data we sign and push to the chain (not sure what this use case would be, and why we'd verify sigs on chain).

I'm not necessarily for or against this idea, but I'd like to hear why @Villaquiranm would need signature verification on-chain through native methods 👀

For reference, Ethereum has native methods in Solidity for deriving the address from the ECDSA signature, but not for verifying it (there are library contracts that people can import and use) -- which supports the argument that sig verification is something that should be done off-chain

Gno2D2 · 2025-01-08T08:40:27Z

🛠 PR Checks Summary

🔴 Maintainers must be able to edit this pull request (more info)

Manual Checks (for Reviewers):

IGNORE the bot requirements for this PR (force green CI check)

✅ Automated Checks (for Contributors):

🔴 Maintainers must be able to edit this pull request (more info)

☑️ Contributor Actions:

Fix any issues flagged by automated checks.
Follow the Contributor Checklist to ensure your PR is ready for review.
- Add new tests, or document why they are unnecessary.
- Provide clear examples/screenshots, if necessary.
- Update documentation, if required.
- Ensure no breaking changes, or include BREAKING CHANGE notes.
- Link related issues/PRs, where applicable.

☑️ Reviewer Actions:

Complete manual checks for the PR, including the guidelines and additional checks if applicable.

📚 Resources:

Debug

Automated Checks
Maintainers must be able to edit this pull request (more info)
If
🟢 Condition met
└── 🟢 The pull request was created from a fork (head branch repo: TERITORI/gno)
Then
🔴 Requirement not satisfied
└── 🔴 Maintainer can modify this pull request
Manual Checks
**IGNORE** the bot requirements for this PR (force green CI check)
If
🟢 Condition met
└── 🟢 On every pull request
Can be checked by

Any user with comment edit permission

Villaquiranm requested a review from thehowl as a code owner September 9, 2024 20:09

github-actions bot assigned Villaquiranm Sep 9, 2024

github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Sep 9, 2024

Villaquiranm mentioned this pull request Sep 9, 2024

Make Gnorkle Agents Gas-Less #2777

Open

Villaquiranm force-pushed the feat/verifySignature branch 3 times, most recently from 76ec144 to 74ed842 Compare September 11, 2024 06:56

feat: verifySignature function

707ba33

Villaquiranm force-pushed the feat/verifySignature branch from 74ed842 to 707ba33 Compare September 11, 2024 07:33

Villaquiranm requested review from jaekwon, deelawn and mvertes as code owners September 11, 2024 07:33

michelleellen mentioned this pull request Sep 17, 2024

Minutes: Public Dev Calls [every Wednesday] gnolang/meetings#37

Open

zxxma mentioned this pull request Sep 25, 2024

Teritori's Builder Journey gnolang/hackerspace#7

Open

Kouteki added the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Oct 3, 2024

Villaquiranm added 3 commits October 10, 2024 22:34

Merge branch 'master' of github.com:gnolang/gno into feat/verifySigna…

4014826

…ture

Fix tests

6de7a08

more tests

06d4232

Merge branch 'master' of github.com:gnolang/gno into feat/verifySigna…

43e14eb

…ture

Villaquiranm force-pushed the feat/verifySignature branch from 98a6ec0 to 43e14eb Compare October 17, 2024 19:46

notJoon approved these changes Oct 18, 2024

View reviewed changes

notJoon removed the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Oct 18, 2024

zivkovicmilos requested changes Jan 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: verifySignature function #2776

feat: verifySignature function #2776

Villaquiranm commented Sep 9, 2024 •

edited

Loading

codecov bot commented Sep 9, 2024 •

edited

Loading

notJoon commented Oct 10, 2024

Villaquiranm commented Oct 11, 2024 •

edited

Loading

notJoon left a comment •

edited

Loading

thehowl commented Oct 23, 2024

zivkovicmilos left a comment •

edited

Loading

zivkovicmilos Jan 8, 2025

zivkovicmilos Jan 8, 2025

zivkovicmilos Jan 8, 2025

zivkovicmilos Jan 8, 2025

zivkovicmilos Jan 8, 2025

zivkovicmilos Jan 8, 2025

zivkovicmilos Jan 8, 2025

zivkovicmilos Jan 8, 2025

zivkovicmilos commented Jan 8, 2025

Gno2D2 commented Jan 8, 2025

✅ Automated Checks (for Contributors):

☑️ Contributor Actions:

☑️ Reviewer Actions:

📚 Resources:

If

Then

If

Can be checked by

		)

		const DefaultAccount_Seed = "source bonus chronic canvas draft south burst lottery vacant surface solve popular case indicate oppose farm nothing bullet exhibit title speed wink action roast"

feat: verifySignature function #2776

Are you sure you want to change the base?

feat: verifySignature function #2776

Conversation

Villaquiranm commented Sep 9, 2024 • edited Loading

codecov bot commented Sep 9, 2024 • edited Loading

Codecov Report

notJoon commented Oct 10, 2024

Villaquiranm commented Oct 11, 2024 • edited Loading

notJoon left a comment • edited Loading

Choose a reason for hiding this comment

thehowl commented Oct 23, 2024

zivkovicmilos left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zivkovicmilos commented Jan 8, 2025

Gno2D2 commented Jan 8, 2025

🛠 PR Checks Summary

Manual Checks (for Reviewers):

✅ Automated Checks (for Contributors):

☑️ Contributor Actions:

☑️ Reviewer Actions:

📚 Resources:

If

Then

If

Can be checked by

Villaquiranm commented Sep 9, 2024 •

edited

Loading

codecov bot commented Sep 9, 2024 •

edited

Loading

Villaquiranm commented Oct 11, 2024 •

edited

Loading

notJoon left a comment •

edited

Loading

zivkovicmilos left a comment •

edited

Loading