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

API Harmonization #15

Merged
merged 6 commits into from May 9, 2022
Merged

API Harmonization #15

merged 6 commits into from May 9, 2022

Conversation

theosirian
Copy link
Collaborator

This PR as discussed internally tries to harmonize all siwe libraries to a similar behavior across all different languages.

  • API uniformization across libraries
  • Improve message syntax checking
  • Improve error messages
  • Improve tests and adding siwe-ts test vectors

Signed-off-by: Tiago Nascimento <tiago.nascimento@spruceid.com>
- Fixed test cases parsing to follow json unmarshalling number to float64
- Update submodule to api harmonization tests
- URL and Domain validation
- Remove spaces from matching groups in URI
- Modify return type on `Verify`
- Add domain, nonce, timestamp optional parameters to `Verify`
Signed-off-by: Tiago Nascimento <tiago.nascimento@spruceid.com>
Copy link

@awoie awoie left a comment

Choose a reason for hiding this comment

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

lgtm in general, just a few comments and perhaps we should add some doc to the exported functions since we introduced a bunch of optional params.

siwe.go Outdated
return validateURI, nil
}

func InitMessage(domain, address, uri, version string, options map[string]interface{}) (*Message, error) {
Copy link

Choose a reason for hiding this comment

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

just wondering why issued at, nonce and expiration are in the options map since those are mandatory and should be always present?

Copy link

Choose a reason for hiding this comment

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

do we need to provide the version since it is always 1?

Copy link

Choose a reason for hiding this comment

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

can we provide inline doc for public / exported functions?

Choose a reason for hiding this comment

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

+1 on @awoie suggestion on not making version string a required parameter here.

}
validateResources[i] = *validateResource
}
result["resources"] = validateResources
}

return result, nil
Copy link

Choose a reason for hiding this comment

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

can we provide inline doc for public / exported functions -> ParseMessage(...): Message?

@@ -221,7 +303,7 @@ func (m *Message) VerifyEIP191(signature string) (*ecdsa.PublicKey, error) {
return pkey, nil
}

func (m *Message) Verify(signature string, nonce *string, timestamp *time.Time) (*ecdsa.PublicKey, error) {
func (m *Message) Verify(signature string, domain *string, nonce *string, timestamp *time.Time) (*ecdsa.PublicKey, error) {
Copy link

Choose a reason for hiding this comment

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

can we provide inline doc for public / exported functions?

@@ -281,7 +332,7 @@ func parsingPositive(t *testing.T, cases map[string]interface{}) {
}
}

func validationNegative(t *testing.T, cases map[string]interface{}) {
func verificationNegative(t *testing.T, cases map[string]interface{}) {
Copy link

Choose a reason for hiding this comment

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

more like a general question and not related for this PR but would it make sense to move those tests to the global test vectors as well at some point?

Copy link
Member

@sbihel sbihel left a comment

Choose a reason for hiding this comment

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

Nothing more to say than Oliver's comments

Signed-off-by: Tiago Nascimento <tiago.nascimento@spruceid.com>
Signed-off-by: Tiago Nascimento <tiago.nascimento@spruceid.com>
Signed-off-by: Tiago Nascimento <tiago.nascimento@spruceid.com>
@theosirian theosirian merged commit a36f2a0 into main May 9, 2022
@sbihel sbihel deleted the feat/api-harmonization branch May 10, 2022 12:35
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.

None yet

5 participants