feat: add multi-account support #3433
Conversation
ninabarbakadze
force-pushed
the
cal/multi-accounts
branch
from
eed5726
to
a7ce130
Compare
pkg/user/tx_client.go
Outdated
| accNum, seqNum, err := QueryAccount(ctx, conn, encCfg.InterfaceRegistry, addr) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("querying account: %w", err) | ||
| } |
There was a problem hiding this comment.
just to record something, this mech forces each key in the keyring to also be an account with state. In general, this is a desired behavior, however if a user adds a new key to the keyring, now they will be unable setup the txclient
There was a problem hiding this comment.
They should be able to add keys after setting up the tx client. If it's the first time the key is being used, the txclient will automatically query for the sequence number etc... before submitting the transactions.
…app into cal/multi-accounts
…app into cal/multi-accounts
cmwaters
requested review from
rootulp and
evan-forbes
and removed request for
a team
WalkthroughThe changes involve updating various test files and the Changes
Assessment against linked issues
The changes comprehensively address the objectives from the linked issue by enhancing the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Outside diff range and nitpick comments (2)
pkg/user/signer.go (1)
Line range hint
88-110: Improve error handling and messaging inCreatePayForBlobs.The
CreatePayForBlobsmethod could improve its error handling by providing more detailed error messages, especially when an account is not found or when there are issues with transaction signing or encoding. Consider enhancing the error messages to include more context about the operation being performed and the specific failure point.test/util/testnode/node_interaction_api.go (1)
Line range hint
242-255: Refactor thePostDatamethod to use the newTxClientfor transaction submission.- signer, err := user.NewSigner(c.Keyring, c.TxConfig, c.ChainID, appconsts.LatestVersion, user.NewAccount(account, acc, seq)) + txClient, err := user.NewTxClient(c.Keyring, c.TxConfig, c.ChainID, appconsts.LatestVersion, user.NewAccount(account, acc, seq)) if err != nil { return nil, err } - blobTx, _, err := signer.CreatePayForBlobs(account, []*blob.Blob{b}, opts...) + blobTx, _, err := txClient.CreatePayForBlobs(account, []*blob.Blob{b}, opts...) if err != nil { return nil, err }This change aligns the
PostDatamethod with the new architecture whereTxClienthandles transaction submissions, improving consistency and leveraging the new capabilities ofTxClient.
| // Signer is struct for building and signing Celestia transactions | ||
| // It supports multiple accounts wrapping a Keyring. | ||
| // NOTE: All transactions may only have a single signer | ||
| // Signer is not thread-safe. | ||
| type Signer struct { | ||
| keys keyring.Keyring | ||
| address sdktypes.AccAddress | ||
| enc client.TxConfig | ||
| grpc *grpc.ClientConn | ||
| pk cryptotypes.PubKey | ||
| chainID string | ||
| accountNumber uint64 | ||
| keys keyring.Keyring | ||
| enc client.TxConfig | ||
| chainID string | ||
| // FIXME: the signer is currently incapable of detecting an appversion | ||
| // change and could produce incorrect PFBs if it the network is at an | ||
| // appVersion that the signer does not support | ||
| appVersion uint64 | ||
|
|
||
| mtx sync.RWMutex | ||
| // how often to poll the network for confirmation of a transaction | ||
| pollTime time.Duration | ||
| // the signers local view of the sequence number | ||
| localSequence uint64 | ||
| // the chains last known sequence number | ||
| networkSequence uint64 | ||
| // gasMultiplier is used to increase gas limit as it is sometimes underestimated | ||
| gasMultiplier float64 | ||
| // lookup map of all pending and yet to be confirmed outbound transactions | ||
| outboundSequences map[uint64]struct{} | ||
| // a reverse map for confirming which sequence numbers have been committed | ||
| reverseTxHashSequenceMap map[string]uint64 | ||
| // set of accounts that the signer can manage. Should match the keys on the keyring | ||
| accounts map[string]*Account | ||
| addressToAccountMap map[string]string |
There was a problem hiding this comment.
Refactor the Signer struct to handle app version changes dynamically.
The current implementation of the Signer struct includes a FIXME comment about its inability to handle app version changes dynamically. This could lead to incorrect transaction signing if the app version changes. Consider implementing a mechanism to update the appVersion dynamically or fetch it as needed to ensure the Signer remains accurate across version changes.
There was a problem hiding this comment.
Turned this into an issue because I don't believe we have one currently
| func (s *TxClient) checkAccountLoaded(ctx context.Context, account string) error { | ||
| if _, exists := s.signer.accounts[account]; exists { | ||
| return nil | ||
| } | ||
| record, err := s.signer.keys.Key(account) | ||
| if err != nil { | ||
| return fmt.Errorf("trying to find account %s on keyring: %w", account, err) | ||
| } | ||
| addr, err := record.GetAddress() | ||
| if err != nil { | ||
| return fmt.Errorf("retrieving address from keyring: %w", err) | ||
| } | ||
| accNum, sequence, err := QueryAccount(ctx, s.grpc, s.registry, addr) | ||
| if err != nil { | ||
| return fmt.Errorf("querying account %s: %w", account, err) | ||
| } | ||
| return s.signer.AddAccount(NewAccount(account, accNum, sequence)) |
There was a problem hiding this comment.
Optimize account checking and querying logic.
This function performs multiple operations that could be streamlined. For example, querying the account could be moved to a separate method to improve readability and reusability.
+ func (s *TxClient) queryAccount(ctx context.Context, account string) error {
+ // Implementation of account querying logic
+ }
- accNum, sequence, err := QueryAccount(ctx, s.grpc, s.registry, addr)
+ err := s.queryAccount(ctx, account)Committable suggestion was skipped due low confidence.
| func WithDefaultAddress(address sdktypes.AccAddress) Option { | ||
| return func(c *TxClient) { | ||
| record, err := c.signer.keys.KeyByAddress(address) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| c.defaultAccount = record.Name | ||
| } |
There was a problem hiding this comment.
Consider handling errors more gracefully instead of using panic.
Using panic for error handling in production code, especially in a library, can lead to abrupt termination of applications that use this library. It would be better to return an error and let the caller decide how to handle it.
- panic(err)
+ return nil, errCommittable suggestion was skipped due low confidence.
| txClient *user.TxClient | ||
| balance uint64 | ||
| latestHeight uint64 | ||
| lastUpdated time.Time | ||
| subaccounts map[string]*user.Signer | ||
| accountIndex int | ||
| addressMap map[string]string |
There was a problem hiding this comment.
Refactor to use the new TxClient for transaction management.
- am.txClient, err = user.SetupTxClient(ctx, am.keys, am.conn, am.encCfg, user.WithDefaultAccount(masterAccName), user.WithPollTime(am.pollTime))
+ txClient, err := user.NewTxClient(ctx, am.keys, am.conn, am.encCfg, user.WithDefaultAccount(masterAccName), user.WithPollTime(am.pollTime))
if err != nil {
return err
}
am.txClient = txClientThis change updates the AccountManager to instantiate a TxClient using the new constructor, aligning with the updated transaction management architecture.
Also applies to: 149-155, 175-186, 213-214, 239-239, 247-257, 282-282
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| txClient *user.TxClient | |
| balance uint64 | |
| latestHeight uint64 | |
| lastUpdated time.Time | |
| subaccounts map[string]*user.Signer | |
| accountIndex int | |
| addressMap map[string]string | |
| txClient, err := user.NewTxClient(ctx, am.keys, am.conn, am.encCfg, user.WithDefaultAccount(masterAccName), user.WithPollTime(am.pollTime)) | |
| if err != nil { | |
| return err | |
| } | |
| am.txClient = txClient |
| msgs, signer := tt.msgFunc() | ||
| txClient, err := user.SetupTxClient(s.cctx.GoContext(), s.cctx.Keyring, s.cctx.GRPCClient, s.ecfg, user.WithDefaultAccount(signer)) | ||
| require.NoError(t, err) | ||
| res, err := signer.SubmitTx(s.cctx.GoContext(), msgs, blobfactory.DefaultTxOpts()...) | ||
| res, err := txClient.SubmitTx(s.cctx.GoContext(), msgs, blobfactory.DefaultTxOpts()...) |
There was a problem hiding this comment.
Update the test setup to use the new TxClient.
- txClient, err := user.SetupTxClient(s.cctx.GoContext(), s.cctx.Keyring, s.cctx.GRPCClient, s.ecfg, user.WithDefaultAccount(signer))
+ txClient, err := user.NewTxClient(s.cctx.GoContext(), s.cctx.Keyring, s.cctx.GRPCClient, s.ecfg, user.WithDefaultAccount(signer))
require.NoError(t, err)This change ensures that the test suite uses the new TxClient constructor, maintaining consistency with the updated transaction management system.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| msgs, signer := tt.msgFunc() | |
| txClient, err := user.SetupTxClient(s.cctx.GoContext(), s.cctx.Keyring, s.cctx.GRPCClient, s.ecfg, user.WithDefaultAccount(signer)) | |
| require.NoError(t, err) | |
| res, err := signer.SubmitTx(s.cctx.GoContext(), msgs, blobfactory.DefaultTxOpts()...) | |
| res, err := txClient.SubmitTx(s.cctx.GoContext(), msgs, blobfactory.DefaultTxOpts()...) | |
| msgs, signer := tt.msgFunc() | |
| txClient, err := user.NewTxClient(s.cctx.GoContext(), s.cctx.Keyring, s.cctx.GRPCClient, s.ecfg, user.WithDefaultAccount(signer)) | |
| require.NoError(t, err) | |
| res, err := txClient.SubmitTx(s.cctx.GoContext(), msgs, blobfactory.DefaultTxOpts()...) |
| startingShare: 56, | ||
| endingShare: 58, |
There was a problem hiding this comment.
[no change needed] do you have any idea why these blob shares got move over by one share? Perhaps something in the tx namespace occupied one extra share?
There was a problem hiding this comment.
Yeah I was actually curious that this needed to change. I looked through the code and couldn't see why the PFBs would take up more space. One thing I think we should add if we don't have already is regression tests to the square / share construction to make sure that breaking changes are caught
celestia-bot
requested review from
a team and
ninabarbakadze
and removed request for
a team
Update celestia-node to use celestia-app v2.0.0. A few changes that were needed: - celestia-app v1.x had a shares package. celestia-app v2.x uses the shares package from go-square. - celestia-app v1.x had a blob.types package with `CreateCommitment`. celestia-app v2.x uses `CreateCommitment` from the go-square inclusion package. - I had to update extended header verification to allow `header.Version.App = 2`. Added unit tests. - celestia-app v1.x had a lot of functionality included in the `signer`. celestia-app v2.x split a `txClient` from the `signer`. See: celestiaorg/celestia-app#3433 - ~~I had to update `core_access.go` a lot. Mostly inspired by #3451 ## Testing I ran a [script](https://gist.github.com/rootulp/73ee382b4d533cb9da27fc675e9047c0) with: celestia-app v2.0.0-rc2 and configured it to upgrade at block height 3. celestia-node (built from this PR) continued to work: ``` 2024-07-09T18:13:27.040-0400 INFO header/store store/store.go:367 new head {"height": 2, "hash": "8776AEAF4114BD7E88E8DEC38445720D0BD857335BED99649957A43BB845EC87"} 2024-07-09T18:13:38.065-0400 INFO header/store store/store.go:367 new head {"height": 3, "hash": "63D5C64521A964290BD21658314DDF60146AE419FE99026003048F74D2886B35"} 2024-07-09T18:13:49.093-0400 INFO header/store store/store.go:367 new head {"height": 4, "hash": "FC7900918E716697A7CD6D9A4865B261F3E71D181F366E765AA53CF475223F9A"} ```
Update celestia-node to use celestia-app v2.0.0. A few changes that were needed: - celestia-app v1.x had a shares package. celestia-app v2.x uses the shares package from go-square. - celestia-app v1.x had a blob.types package with `CreateCommitment`. celestia-app v2.x uses `CreateCommitment` from the go-square inclusion package. - I had to update extended header verification to allow `header.Version.App = 2`. Added unit tests. - celestia-app v1.x had a lot of functionality included in the `signer`. celestia-app v2.x split a `txClient` from the `signer`. See: celestiaorg/celestia-app#3433 - ~~I had to update `core_access.go` a lot. Mostly inspired by celestiaorg#3451 ## Testing I ran a [script](https://gist.github.com/rootulp/73ee382b4d533cb9da27fc675e9047c0) with: celestia-app v2.0.0-rc2 and configured it to upgrade at block height 3. celestia-node (built from this PR) continued to work: ``` 2024-07-09T18:13:27.040-0400 INFO header/store store/store.go:367 new head {"height": 2, "hash": "8776AEAF4114BD7E88E8DEC38445720D0BD857335BED99649957A43BB845EC87"} 2024-07-09T18:13:38.065-0400 INFO header/store store/store.go:367 new head {"height": 3, "hash": "63D5C64521A964290BD21658314DDF60146AE419FE99026003048F74D2886B35"} 2024-07-09T18:13:49.093-0400 INFO header/store store/store.go:367 new head {"height": 4, "hash": "FC7900918E716697A7CD6D9A4865B261F3E71D181F366E765AA53CF475223F9A"} ```
Closes: #3259
This PR adds support for using multiple kerying keys with a single Signer. It splits signer into a purely offline signer for signing messages and the txclient responsible for tx submission and handling