Feat: Add safe flag #71
Conversation
evm/generators.go
Outdated
| code = string(generatedCode) | ||
| } | ||
|
|
||
| return code, nil | ||
| codeImportsFixed := strings.ReplaceAll(code, "github.com/quan8/go-ethereum", "github.com/ethereum/go-ethereum") |
There was a problem hiding this comment.
Why is this necessary? This looks like a problem in your local development environment that you are pushing a fix for to all users.
There was a problem hiding this comment.
You are totally right, this is not necessary. It was introduced in early stage of development to make it work. Removing it.
There was a problem hiding this comment.
@zomglings: I've updated the code to remove this import. But it keeps importing the wrong ones:
"github.com/quan8/go-ethereum/common/math"
"github.com/quan8/go-ethereum/crypto"
There was a problem hiding this comment.
Interesting. I wonder why that is. Could it be an LLM issue?
There was a problem hiding this comment.
Imports fixed! It was missing from appending to t.Specs struct 😄
evm/generators.go
Outdated
| return CreateSafeProposal(client, key, safeAddress, factoryAddress, safeCreateCallTxData, value, txServiceBaseUrl, OperationType(safeOperation)) | ||
| } | ||
|
|
||
| func GenerateProperSalt(from common.Address) ([32]byte, error) { |
There was a problem hiding this comment.
I don't think we need this anymore, since we are not using the ImmutableCreate2Factory anymore?
There was a problem hiding this comment.
We still need to pass a salt to call safeCreate2 on the CreateCall contract. But you are right; to simplify this function, we no longer need to append the caller's address at the beginning of the salt.
Would you like to update it, or can we keep it with ImmutableCreate2Factory compatibility if we want to use it in the future?
There was a problem hiding this comment.
I'd rather just take a --salt on the deploy --safe command.
Take salt as an argument from the caller, basically.
I don't like to force a particular salt generation strategy on the user.
There was a problem hiding this comment.
In this case, do you think it is a problem to keep it that way?
Because in the actual implementation, if you use --salt, it will override this one.
The autogeneration only happens if the users do not specify a --salt
There was a problem hiding this comment.
@karacurt : I prefer to remove it - the requirement that the salt be prefixed with the caller address is an optional constraint implemented by that particular CREATE2 factory. Other CREATE2 factories will have other means to prevent spoofs, and some may not even put any protections in place. Using that as a default salt just feelds weird to me - and it actually makes it harder to use Safe with ImmutableCreate2Factory!
I would have one of two expectations for --salt. Either:
- Use a random salt, or
- Make
--salta required argument
I prefer to make it required.
There was a problem hiding this comment.
So, do we need to make a CLI to generate a random salt? I think I didn't get this part.
I am ok with removing the salt standard, but I think it would be useful to have a autogenerated salt if --salt not passed
evm/generators.go
Outdated
| if err != nil { | ||
| return fmt.Errorf("failed to get ABI: %v", err) | ||
| } | ||
| // TODO: this is a workaround to match the original method name in the ABI, need to fix to work with every method name |
There was a problem hiding this comment.
I will try my best to explain here. If it's not clear, maybe we should pair for a better understanding:
I found out that abi.Pack("methodName", args...) is case sensitive. It only works if the methodName has the exact case-sensitive name as in the abi. Unfortunately, the parser modifies the method name to UpperCamelCase.
I've tried a similar method of calling the transactor with NoSend flag equal to true. However, it does not work because it simulates the transaction against the blockchain. In this case, the dev wallet is the from address, and it needs to be the safeAddress. I couldn't find an option to the the from address as a different address from the caller.
I think there is probably a more straightforward solution, so I left this TODO: comment so we could work together on this.
There was a problem hiding this comment.
Could we put the original method name into the object that gets sent to the template?
There was a problem hiding this comment.
Just fixed the problem using session. object. It create the calldata without sending it
karacurt
force-pushed
the
karacurt-temp
branch
from
49d21ae to
5007b25
Compare
This PR adds
safefunctionality toseer. Details:--safeflag, it will propose a transaction to the safe.safeflag is used along withdeploy, it will propose aDELEGATECALLcallingperformCreate2function in theCreateCallcontract.