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: Onchain Identifier via transaction data and calldata fields #1059

Open
wants to merge 9 commits into
base: development
Choose a base branch
from

Conversation

DaniSomoza
Copy link
Contributor

@DaniSomoza DaniSomoza commented Nov 24, 2024

What it solves

Resolves #1056

How this PR fixes it

Append the on chain identifier to the transaction data field and in the calldata field

Examples

Chain: Sepolia

Safe deployment of a Safe Account (v1.3.0) in Sepolia using the protocol-kit

Safe Deployed: 0x096B2222AE1600D0074d35a8A6BC58FF04AB2384

Transaction Hash: 0xe0192eedd1fc2d06be0561d57380d610dd6d162af0f3cfbd6c08f9062d738761

On-chain Identifier: 5afe006363643438383836663461336661366162653539646561393238653366

Metadata included in the On-chain Identifier:

  • Identifier Prefix: 5afe
  • Identifier Version 00
  • Project: Test Dapp SDK hashed: 6363643438383836663461336661366162653539
  • Platform: Web: hashed: 646561
  • Tool: protocol-kit: hashed: 393238
  • Tool version: 5.0.4: hashed: 653366

Safe transaction of a Safe Account (v1.3.0) in Sepolia using the protocol-kit

Safe used: 0x096B2222AE1600D0074d35a8A6BC58FF04AB2384

the transaction is a rejection tx.

Transaction Hash: 0x0256ef4a2e21c5b529c2b60b3e48327261508d685a24e331788e2ee64275f9b0

On-chain Identifier: 5afe006363643438383836663461336661366162653539646561393238653366

Metadata included in the On-chain Identifier:

  • Identifier Prefix: 5afe
  • Identifier Version 00
  • Project: Test Dapp SDK hashed: 6363643438383836663461336661366162653539
  • Platform: Web: hashed: 646561
  • Tool: protocol-kit: hashed: 393238
  • Tool version: 5.0.4: hashed: 653366

Safe deployment + batch of transactions of a 4337 Safe Account (v1.4.1) in Sepolia using the relay-kit

Safe used: 0xf5E1c84b12763A70c79679ad00d34F0769Be57Cb

the transaction is a deployment of the Safe and a batch of 2 USDC transfers (amount: 0.01 USDC).

Transaction Hash: 0xd8a1a85c7b5ecffe62ba17d02d40f76043566beacb291279aef18242068e1a35

Transaction jiffyscan Link: https://jiffyscan.xyz/userOpHash/0xe1bd646760cf53f0332fec03ef4c094d8132462b1a4258069d7fa8ff08b96657?network=sepolia

On-chain Identifier: 5afe006363643438383836663461336661366162653539646561393063666133

Metadata included in the On-chain Identifier:

  • Identifier Prefix: 5afe
  • Identifier Version 00
  • Project: Test Dapp SDK hashed: 6363643438383836663461336661366162653539
  • Platform: Web: hashed: 646561
  • Tool: relay-kit: hashed: 393063
  • Tool version: 3.2.4: hashed: 666133

@coveralls
Copy link

coveralls commented Nov 24, 2024

Pull Request Test Coverage Report for Build 12254974241

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 7 of 12 (58.33%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.3%) to 75.199%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/relay-kit/src/packs/safe-4337/utils/getRelayKitVersion.ts 2 3 66.67%
packages/relay-kit/src/packs/safe-4337/Safe4337Pack.ts 5 9 55.56%
Totals Coverage Status
Change from base Build 12008934309: -0.3%
Covered Lines: 795
Relevant Lines: 985

💛 - Coveralls

@yagopv yagopv linked an issue Nov 25, 2024 that may be closed by this pull request
@DaniSomoza DaniSomoza changed the title Onchain trackid via transaction data field Onchain indentifier via transaction data and calldata fields Nov 28, 2024
@DaniSomoza DaniSomoza changed the title Onchain indentifier via transaction data and calldata fields feat: Onchain Identifier via transaction data and calldata fields Nov 28, 2024
@DaniSomoza DaniSomoza requested a review from yagopv November 28, 2024 14:01
@yagopv yagopv linked an issue Dec 2, 2024 that may be closed by this pull request

/**
* Generates an on-chain identifier for tracking transactions on the blockchain.
* This identifier includes hased metadata such as the project name, platform, tool, and tool version.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* This identifier includes hased metadata such as the project name, platform, tool, and tool version.
* This identifier includes hashed metadata such as the project name, platform, tool, and tool version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

const { provider, signer, isL1SafeSingleton, contractNetworks } = config
const { provider, signer, isL1SafeSingleton, contractNetworks, onchainAnalitics } = config

if (onchainAnalitics?.project) {
Copy link
Member

Choose a reason for hiding this comment

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

Protocol Kit Version is hardcoded here 🙃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done! I added a getProtocolKitVersion and a getRelayKitVersion to automatically get the version of the package from the package.json

const safeDeploymentBatch = await this.createTransactionBatch(
transactions,
transactionOptions,
true // include the on chain identifier
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this. Why including it explicetely? If the private variable exists then we can include it right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It’s because we don’t always know where the batch created with the createTransactionBatch function will be used. Automatically appending the trackId could cause issues in transactions that aren’t meant to be executed directly.

@@ -50,6 +54,8 @@ export type Safe4337InitOptions = {
}
options: ExistingSafeOptions | PredictedSafeOptions
paymasterOptions?: PaymasterOptions
// on-chain analitics
Copy link
Member

Choose a reason for hiding this comment

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

The variable name does the same as the comment right? and the other props has no comments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true, removed! 🙏

@@ -61,6 +67,8 @@ export type Safe4337Options = {
entryPointAddress: string
safe4337ModuleAddress: string
safeWebAuthnSharedSignerAddress?: string
// on-chain analitics
Copy link
Member

@yagopv yagopv Dec 2, 2024

Choose a reason for hiding this comment

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

Same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true, removed! 🙏

@@ -22,7 +27,7 @@ interface Config {

const config: Config = {
RPC_URL: sepolia.rpcUrls.default.http[0],
DEPLOYER_ADDRESS_PRIVATE_KEY: '<DEPLOYER_ADDRESS_PRIVATE_KEY>',
DEPLOYER_ADDRESS_PRIVATE_KEY: SIGNER_ADDRESS_PRIVATE_KEY!,
Copy link
Member

Choose a reason for hiding this comment

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

Why this comming from the dotenv while the other playgrounds don't. I will address this in a separate PR if it's necessary and align all playgrounds

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

onchainAnalitics.project,
onchainAnalitics.platform,
'relay-kit',
'3.2.4'
Copy link
Member

Choose a reason for hiding this comment

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

Hardcoded version ? Should be coming from package.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done! I added a getProtocolKitVersion and a getRelayKitVersion to automatically get the package version from the package.json

Comment on lines +831 to +833
getOnchainIdentifier(): string {
return this.#onchainIdentifier
}
Copy link
Member

Choose a reason for hiding this comment

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

This can be probably this.protocolKit.getOnchainIdentifier right?

Copy link
Contributor Author

@DaniSomoza DaniSomoza Dec 10, 2024

Choose a reason for hiding this comment

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

the onchainIdentifier set in the protocol-kit is different from the one in the relay-kit.

That’s why we need separate functions for each, like protocolKit.getOnchainIdentifier and relayKit.getOnchainIdentifier

the tool and toolVersion used to generate the identifier are different for each case.

* @param {string} toolVersion - The version of the tool used to generate the transaction.
* @returns {string} A string representing the on-chain identifier, composed of multiple hashed segments.
*/
function generateOnChainIdentifier(
Copy link
Member

Choose a reason for hiding this comment

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

Just a small suggestion. Could this function receive an object instead? It would allow to share more understandable code snippets in the docs.

generateOnChainIdentifier({
  project: 'asdfasdf',
  // ...
})

better than

generateOnChainIdentifier('asdfasdf', ...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

generateOnChainIdentifier function updated to accept an object as suggested sir 🫡

Thanks!

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.

[Onchain tracking] Implementation [Onchain tracking] PoC
4 participants