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

feat: EIP-1271 #301

Merged
merged 7 commits into from
Feb 5, 2024
Merged

feat: EIP-1271 #301

merged 7 commits into from
Feb 5, 2024

Conversation

chris13524
Copy link
Member

@chris13524 chris13524 commented Jan 19, 2024

Description

Resolves #145

Several improvements should be made in another iteration, but these are not urgent: #345

Remaining work:

  • Update provider URL to be dynamic based on chain ID
  • Merge WalletConnectRust PR & use versioned

How Has This Been Tested?

Automated tests

Due Diligence

  • Breaking change
  • Requires a documentation update
  • Requires a e2e/integration test update

@chris13524 chris13524 self-assigned this Jan 19, 2024
@chris13524 chris13524 marked this pull request as ready for review January 31, 2024 23:00
@chris13524 chris13524 requested review from Elyniss, geekbrother, arein, xav and nopestack and removed request for geekbrother January 31, 2024 23:00
Copy link

@Elyniss Elyniss left a comment

Choose a reason for hiding this comment

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

Was this tested on staging somewhere? Or just in WCRust repo?

@@ -7,7 +7,8 @@ allow = [
"Unlicense",
"BSD-3-Clause",
"0BSD",
"ISC"
"ISC",
"CC0-1.0"
]
Copy link

Choose a reason for hiding this comment

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

Why was this updated?

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems like a dependency changed and this license is actually being used. A few weeks ago the checker had me remove it because it was unused, but now it wanted me to add back. Not sure what happened.

This license is a public domain license so we can allow it.

@chris13524
Copy link
Member Author

chris13524 commented Feb 5, 2024

Was this tested on staging somewhere? Or just in WCRust repo?

I have not tested EIP-1271 in Notify Server, just in WalletConnectRust. The changes here are extremely minimal so don't think testing was necessary. EOAs still work (according to tests), so if smart accounts upport doesn't work we can fix in follow-up.

pub struct MockGetRpcUrl;
impl GetRpcUrl for MockGetRpcUrl {
fn get_rpc_url(&self, _: String) -> Option<Url> {
None
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work in tests as it's always None?

Copy link
Member Author

Choose a reason for hiding this comment

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

EIP-1271 is not tested by Notify Server tests (however it should be in the future). So since this function will never be called, it's ok to return None.

@chris13524 chris13524 merged commit 1c6c3ab into main Feb 5, 2024
13 of 17 checks passed
@chris13524 chris13524 deleted the feat/eip1271 branch February 5, 2024 19:16
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.

Smart contract support
3 participants