-
Notifications
You must be signed in to change notification settings - Fork 1
ConvertSubnetToL1 P Chain Tx #153
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
base: make-sdk-uptodate
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the example need more work to be more aligned with the one I proposed
for the namings, probably we can just use pchain instead of transactions/pchain/txs
which should be the alignment with TS here? for the namespaces?
} | ||
} | ||
|
||
func Filter[T any](input []T, f func(T) bool) []T { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you add description for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is straight from CLI utils pkg
return pClient.GetSubnet(ctx, subnetID) | ||
} | ||
|
||
func ConvertToBLSProofOfPossession(publicKey, proofOfPossesion string) (signer.ProofOfPossession, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably the name should be NewSignerProofOfPossession.
Besides this, I don't think it belongs to blockchain/. It most probably belongs to utils/, or pchain/utils/
"github.com/ava-labs/avalanchego/vms/platformvm/txs" | ||
) | ||
|
||
// ConvertSubnetToL1TxParams contains all parameters needed to create a ConvertSubnetToL1Tx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok I agree with this structs to have more compat with TS, will also do in my PR
Address []byte | ||
// Validators are the initial set of L1 validators after the conversion. | ||
Validators []*txs.ConvertSubnetToL1Validator | ||
// Wallet is the wallet used to sign `ConvertSubnetToL1Tx` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe wallet should like outside of this struct, and be a separate argument, as in my PR, so NewConvertSubnetToL1Tx will take the wallet/client, and the params.
With this be have more similarity to TS and I believe it is best organization of the input as the wallet serves
other function, is not part of the tx data
// ConvertSubnetToL1TxParams contains all parameters needed to create a ConvertSubnetToL1Tx | ||
type ConvertSubnetToL1TxParams struct { | ||
// SubnetAuthKeys are the keys used to sign `ConvertSubnetToL1Tx` | ||
SubnetAuthKeys []ids.ShortID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you agree, we can incorporate the simplifications of strings instead of ids.ID, etc, so we are more
similar to TS, and maybe it may be easier for users
} | ||
|
||
func main() { | ||
if err := ConvertL1(); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
"github.com/ethereum/go-ethereum/common" | ||
) | ||
|
||
func ConvertL1() error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you make this very similar to https://github.com/javiertc/avalanche-sdk-test/blob/main/convertSubnetToL1Tx.ts ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this way you will check what is being used and the separate parts: chain, account, wallet, functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can also try to refer to my example with is pretty much based on https://github.com/javiertc/avalanche-sdk-test/blob/main/createSubnetAndChain.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will refactor to be only usign wallet, need to create PR for that
"P-fujixxx", // Replace with actual addresses | ||
} | ||
|
||
network := network.FujiNetwork() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you take the network endpoint from env var as in my example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we want to use env var as opposed to just declaring it in example?
} | ||
|
||
network := network.FujiNetwork() | ||
keychain, err := keychain.NewKeychain(network, privateKeyFilePath, nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets create keychain from string private key in env var
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here switching to wallet
|
||
deployer := txs.NewPublicDeployer(keychain, network) | ||
|
||
wallet, err := wallet.New( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably my new wallet interface is better as it does directly receives SDK objects
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is also a need to add unit for the function, probably in its own file mimicking TS SDK
ConvertSubnetToL1 P-Chain Tx