add prepare create subnet and chain tx #2909
Conversation
felipemadero
requested review from
Tonix517,
jasonatran,
sukantoraymond,
artemanosov,
meaghanfitzgerald and
cwend-ava
| "github.com/ava-labs/avalanchego/wallet/subnet/primary/common" | ||
| ) | ||
|
|
||
| func SendTransaction( |
There was a problem hiding this comment.
Why don't we just move sending transaction to wallet like https://github.com/ava-labs/avalanche-tooling-sdk-go/blob/fef92d1127dc37da50a9307a78663b81a04b971e/wallet/wallet.go#L99?
There was a problem hiding this comment.
I try to be more aligned with TS SDK. Also I believe SendTransaction is somewhat better than Commit.
There was a problem hiding this comment.
I have not issues in moving this into wallet, but I have a slight preference for non OOP style, which is also the TS SDK style for this matter
There was a problem hiding this comment.
anyway, we can have both options (if we find out the TS SDK provides both options)
| "github.com/ava-labs/avalanche-cli/sdk/wallet" | ||
| ) | ||
|
|
||
| func SignTransaction( |
There was a problem hiding this comment.
same here about moving it to wallet object https://github.com/ava-labs/avalanche-tooling-sdk-go/blob/fef92d1127dc37da50a9307a78663b81a04b971e/wallet/wallet.go#L99
There was a problem hiding this comment.
I would move towards TS SDK alignment
| @@ -49,6 +50,20 @@ type Ledger struct { | |||
| LedgerIndices []uint32 | |||
| } | |||
|
|
|||
| func PrivateKeyToAvalancheAccount( | |||
There was a problem hiding this comment.
why do we want this? Why don't we just reuse https://github.com/ava-labs/avalanche-tooling-sdk-go/blob/fef92d1127dc37da50a9307a78663b81a04b971e/wallet/p-chain/examples/convertSubnetToL1Tx.go#L51?
It handles ledger too
There was a problem hiding this comment.
Now that i think about it, we should create wallet object directly from private key / private key path, no need for intermediary keychain / we can abstract it out
There was a problem hiding this comment.
I am trying to align to SDK. they have this account abstraction, very similar to the keychain. Also the function
is there. I believe it is handy. Anyway, not opposite to also having it in wallet. But I would have separate function
for the options:
- ledger
- path
- string
| if err != nil { | ||
| return err | ||
| } | ||
| return kc.AddLedgerIndices(indices) | ||
| } | ||
| return fmt.Errorf("keychain is not ledger enabled") | ||
| } | ||
|
|
||
| func (kc *Keychain) P( |
There was a problem hiding this comment.
can we get address directly from avalanche go Wallet.P() object instead?
There was a problem hiding this comment.
Seems that it will be something like
key := wallet.Keychain.Keychain // This gives you access to the underlying key
pAddress, err := key.P(network.HRP) // network.HRP would be "fuji" for testnet, "avax" for mainnet
There was a problem hiding this comment.
Point of consideration: Should this be in wallet object instead?
There was a problem hiding this comment.
I believe the account can have it. Not opposed to also have it on wallet.
| @@ -283,12 +283,12 @@ func (m *SoftKey) Save(p string) error { | |||
| return os.WriteFile(p, []byte(m.PrivKeyHex()), constants.UserOnlyWriteReadPerms) | |||
| } | |||
|
|
|||
| func (m *SoftKey) P(networkHRP string) (string, error) { | |||
| return address.Format("P", networkHRP, m.privKey.PublicKey().Address().Bytes()) | |||
| func (m *SoftKey) P(network network.Network) (string, error) { | |||
There was a problem hiding this comment.
we already have this P object, do we need another keychain P chain function?
There was a problem hiding this comment.
this is for the key object, it ideally should be used by the keychain in the implementation
| client wallet.Wallet, | ||
| subnetIDStr string, | ||
| vmIDStr string, | ||
| chainName string, | ||
| genesis []byte, | ||
| subnetAuthKeysStr []string, |
There was a problem hiding this comment.
IMO our functions should all be just one argument
e.g.
https://github.com/ava-labs/avalanche-tooling-sdk-go/blob/fef92d1127dc37da50a9307a78663b81a04b971e/wallet/p-chain/txs/convertSubnetToL1Tx.go#L17
There was a problem hiding this comment.
I agree. Two arguments as we talked. the wallet and the params
|
|
||
| // PrepareCreateChainTx creates uncommitted CreateChainTx | ||
| // keychain in wallet will be used to build the transaction | ||
| func PrepareCreateChainTx( |
There was a problem hiding this comment.
also dont think we need to use the same name as typescript SDK. NewCreateChain that we have works better IMO
| "github.com/ava-labs/avalanchego/utils/formatting/address" | ||
| "github.com/ava-labs/avalanchego/vms/platformvm/txs" | ||
| ) | ||
|
|
There was a problem hiding this comment.
thinking this should be on wallet
There was a problem hiding this comment.
There was a problem hiding this comment.
not understanding the comment
| subnetID, err := ids.FromString(subnetIDStr) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error parsing subnet ID: %w", err) | ||
| } | ||
| vmID, err := ids.FromString(vmIDStr) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error parsing vm ID: %w", err) | ||
| } |
There was a problem hiding this comment.
i guess in here we are deciding to let user use string instead of ids.ID. Im not opposed to it
There was a problem hiding this comment.
ok we agree here
Why this should be merged
How this works
How this was tested
How is this documented