-
Notifications
You must be signed in to change notification settings - Fork 20
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
Smart contract sdk support #508
Conversation
Pull main into beta
… np/merge-main-beta
Merge main into beta
… np/smart-contract-sdk-support
… np/smart-contract-sdk-support
…upport Fixes some build & type bugs in smart contracts
…tp/xmtp-react-native into np/smart-contract-sdk-support
… np/smart-contract-sdk-support
self.module = module | ||
self.address = address | ||
self.walletType = walletType | ||
self.chainId = chainId | ||
self.blockNumber = blockNumber |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need the block number to be stored on the signer. That should get resolved at the time of signing, not on instantiation, since the block number keeps incrementing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We store it on the SigningKey
so it only gets passed in from the SigningKey
which means we only have it when the signing key is present which is only when something is being signed. Unfortunately with the bridge this is how it has to be passed. But this and the below shouldn't be exposed to users so it should be an internal matter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I can approve the PR. Still feels wrong to get that block number even one second before we actually sign the message, since it's going to be older than the block the signature was actually generated on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm okay maybe we need to rethink how I have this setup inside the other sdks as well. 🤔
As they all work the same with the block number as part of the signingKey https://github.com/xmtp/xmtp-android/blob/main/library/src/main/java/org/xmtp/android/library/SigningKey.kt#L31-L34 But maybe it should be passed as a param to create instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally it would get pulled from the chain at the same time you are signing the message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another way of thinking about it would be to have a getBlockNumber
method that is only implemented on SCWs, which we could call from our SDKs when we are gathering a smart contract signature.
historySyncUrl?: string | undefined, | ||
walletType?: WalletType | undefined, | ||
chainId?: number | undefined, | ||
blockNumber?: number | undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think we need the block number here either. This only matters when you are actually signing something.
…upport fix: ios type <-> walletType field name mismatch
🎉 This PR is included in version 2.7.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Pulled and cleaned up the SDK pieces from #501