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

docs: update comments in Multisend example #33

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

solarthesis
Copy link
Contributor

What I did

Updated the comments on multisend showing users how to stage transaction without getting caught with the error in case not enough signers are available on local.

fixes: #

As per request #32 (comment)

How I did it

NA

How to verify it

NA

Checklist

  • Passes all linting checks (pre-commit and CI jobs)
  • New test cases have been added and are passing
  • Documentation has been updated
  • PR title follows Conventional Commit standard (will be automatically included in the changelog)

Copy link
Member

@fubuloubu fubuloubu left a comment

Choose a reason for hiding this comment

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

Would like to find a way to refactor the plugin so that this is no longer what you need to do to get a partially signed transaction into the Safe API

cc @antazoey what do you think?

@fubuloubu fubuloubu changed the title Update Comments in Multisend docs: update comments in Multisend example Feb 27, 2024
ape_safe/multisend.py Outdated Show resolved Hide resolved
@fubuloubu fubuloubu enabled auto-merge (squash) February 27, 2024 21:03
@antazoey
Copy link
Member

cc @antazoey what do you think?

I thought the issue was on the core side. In any case, documenting the way it works today is not a bad thing.

@fubuloubu
Copy link
Member

cc @antazoey what do you think?

I thought the issue was on the core side. In any case, documenting the way it works today is not a bad thing.

Issue can be improved with some core updates, but also the assumption if you are executing a transaction directly is that you want to execute it right now, so there should be an easier way to do the second workflow where you don't want to execute it right now that doesn't involve raising an error even though it is actually successfully performing that task

maybe something like SafeAccount.propose(txn: TransactionAPI, **safetx_kwargs) and you can imitate this example with something like:

from ape_safe import multisend
from ape import accounts
# load the safe
safe = accounts.load("my-safe")

ms = multisend.MultiSend()
ms.add(contractA.myMethod1, *call_args)
ms.add(contractB.myMethod2, *call_args)
...  # Add as many calls as desired to execute
ms.add(contractC.myMethod3, *call_args)

safetx = safe.propose(ms.as_transaction())

@antazoey
Copy link
Member

there is a propose cli method you could try

@fubuloubu
Copy link
Member

there is a propose cli method you could try

can that be "upstreamed" into the SafeAccount?

@fubuloubu
Copy link
Member

maybe something like SafeAccount.propose(txn: TransactionAPI, **safetx_kwargs) and you can imitate this example with something like:

from ape_safe import multisend
from ape import accounts
# load the safe
safe = accounts.load("my-safe")

ms = multisend.MultiSend()
ms.add(contractA.myMethod1, *call_args)
ms.add(contractB.myMethod2, *call_args)
...  # Add as many calls as desired to execute
ms.add(contractC.myMethod3, *call_args)

safetx = safe.propose(ms.as_transaction())

p.s. added this here: 9aa25ab

@antazoey
Copy link
Member

antazoey commented Jun 6, 2024

can that be "upstreamed" into the SafeAccount?

Probably!

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.

3 participants