Nexus fix: OwnableValidator & OwnableExecutor integration + tests, + add missing features after viem refactor + fix issues & conflicts +++

bcnmy / biconomy-client-sdk

Biconomy SDK is a plug & play toolkit for dApps to build transaction legos that enable a highly customised one-click experience for their users

MIT License

76 stars 78 forks source link

Nexus fix: OwnableValidator & OwnableExecutor integration + tests, + add missing features after viem refactor + fix issues & conflicts +++ #576

Closed VGabriel45 closed 2 months ago

VGabriel45 commented 2 months ago

PR-Codex overview

This PR focuses on refactoring the handling of signer instead of holder in various modules and updates the related tests. It also introduces new functionalities and makes structural adjustments to improve the codebase.

Detailed summary

Replaced holder with signer in multiple files.
Updated toHolder utility to toSigner.
Modified function signatures and types to use Signer.
Added new module getPreviousModule for retrieving previous module addresses.
Enhanced tests for OwnableExecutor and OwnableValidator.
Adjusted module installation and uninstalling logic.
Updated the package.json dependencies.
Changed addresses in addresses.ts.

The following files were skipped due to too many changes: src/sdk/modules/validators/ownableValidator.test.ts, src/sdk/modules/validators/OwnableValidator.ts

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

livingrockrises commented 2 months ago

conflicts need to be resolved.

livingrockrises commented 2 months ago

conflicts to be resolved

github-actions[bot] commented 2 months ago

size-limit report 📦

Path	Size
core (esm)	11.27 KB (+1.46% 🔺)
core (cjs)	17.34 KB (+1.16% 🔺)
bundler (tree-shaking)	197 B (0%)
paymaster (tree-shaking)	108 B (0%)

livingrockrises commented 2 months ago

ok new conflicts here

joepegler commented 2 months ago

Just a wider point on our modules. I don't like the folder structure any more. It's difficult to know where to look or why the folders are arranged in the way that they are. I also don't like the Base class stuff that we have now. It feels dated, and I'm not sure it even makes sense given how much variety we will have from our modules. I prefer rhinestone flatter / simplified way of organising modules. Can we replicate something similar to what they have done, paired with decorators that can be extended by a nexusClient instead? I realise this might require a different PR

joepegler commented 2 months ago

I've not given this more that a moments thought, but something like...

to

modules >
  smartSessions >
    abi
    constants
    installation
    index
    usage
    decorators
  ownables > 
    abi
    constants
    installation
    index
    usage
    decorators

livingrockrises commented 2 months ago

checks are failing.

livingrockrises commented 2 months ago

I've not given this more that a moments thought, but something like...

to

modules >
  smartSessions >
    abi
    constants
    installation
    index
    usage
    decorators
  ownables > 
    abi
    constants
    installation
    index
    usage
    decorators

folder for each module is ok. most of them will be validators though for which we precisely need these classes because of default and active validator concept.

VGabriel45 commented 2 months ago

getActiveValidationModule

We don't need this, instead we will be migrating to use rhinestone's sdk for module.