Conversation
There was a problem hiding this comment.
Pull request overview
This pull request refactors the wallet and key management architecture to support multiple chain types beyond EVM (e.g., XRPL native, Solana). The refactoring makes the codebase more modular and chain-agnostic by extracting common wallet operations into a shared chains package.
Changes:
- Changed
SavePrivateKeyinterface from accepting*ecdsa.PrivateKeyto acceptingstringfor chain-agnostic key handling - Moved key management functions (AddKeyByPrivateKey, DeleteKey, ExportPrivateKey, ListKeys, ShowKey, AddRemoteSignerKey) from chain provider implementations to a common
chainspackage - Updated all tests and mocks to reflect the new architecture
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| relayer/wallet/wallet.go | Changed SavePrivateKey signature to accept string instead of *ecdsa.PrivateKey |
| relayer/wallet/geth/wallet.go | Updated implementation to convert hex string to ECDSA key internally |
| relayer/wallet/geth/wallet_test.go | Updated tests to pass hex-encoded private keys |
| relayer/chains/keys.go | New file containing chain-agnostic key management functions |
| relayer/chains/signer.go | New file containing LoadSigners helper function |
| relayer/chains/provider.go | Removed key management methods from KeyProvider interface |
| relayer/chains/evm/keys.go | Simplified to only handle mnemonic-based key generation |
| relayer/chains/evm/signer.go | Simplified to delegate to chains.LoadSigners |
| relayer/chains/evm/keys_test.go | Updated tests to use new chains package functions |
| relayer/chains/signer_test.go | Moved and updated test, changed package name |
| relayer/app.go | Added getWallet helper and updated to use chains package functions |
| relayer/app_test.go | Updated mock expectations to work with new architecture |
| internal/relayertest/mocks/wallet.go | Updated mock SavePrivateKey signature |
| internal/relayertest/mocks/chain_provider.go | Removed key management methods from mock |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
769b441 to
81fddd7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 45 out of 46 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if sequence == 0 { | ||
| sequence, err = cp.Client.GetAccountSequenceNumber(ctx, signer.GetAddress()) | ||
| if err != nil { | ||
| log.Error("Get account sequence number error", "retry_count", retryCount, err) | ||
| lastErr = err | ||
| time.Sleep(cp.nonceInterval) | ||
| continue | ||
| } | ||
| } |
There was a problem hiding this comment.
The sequence number is only fetched on the first attempt (when sequence == 0). If the broadcast fails due to a sequence mismatch or the account sequence changes between retries, subsequent retries will use the stale sequence number. Consider refetching the sequence on specific error types or incrementing it on certain failures.
| Account: xrpltypes.Address(signerAddress), | ||
| TransactionType: transaction.OracleSetTx, | ||
| Sequence: uint32(sequence), | ||
| Fee: xrpltypes.XRPCurrencyAmount(12), |
There was a problem hiding this comment.
The hardcoded Fee value of 12 drops may be insufficient for OracleSet transactions, especially when network fees are elevated. Consider making the fee configurable through XRPLChainProviderConfig or using the fee from the config (cp.Config.Fee), which is already validated as required in the Validate method.
| package xrpl | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "path" | ||
|
|
||
| addresscodec "github.com/Peersyst/xrpl-go/address-codec" | ||
| xrplwallet "github.com/Peersyst/xrpl-go/xrpl/wallet" | ||
| toml "github.com/pelletier/go-toml/v2" | ||
|
|
||
| "github.com/bandprotocol/falcon/internal/os" | ||
| "github.com/bandprotocol/falcon/relayer/wallet" | ||
| ) | ||
|
|
||
| var _ wallet.Wallet = &XRPLWallet{} | ||
|
|
||
| const ( | ||
| LocalSignerType = "local" | ||
| RemoteSignerType = "remote" | ||
|
|
||
| SaveMethodSeed = "seed" | ||
| SaveMethodMnemonic = "mnemonic" | ||
|
|
||
| xrplDefaultCoinType = 144 | ||
| ) | ||
|
|
||
| // XRPLWallet manages local and remote signers for a specific chain. | ||
| type XRPLWallet struct { | ||
| Passphrase string | ||
| Signers map[string]wallet.Signer | ||
| HomePath string | ||
| ChainName string | ||
| } | ||
|
|
||
| // NewXRPLWallet creates a new XRPLWallet instance. | ||
| func NewXRPLWallet(passphrase, homePath, chainName string) (*XRPLWallet, error) { | ||
| keyRecordDir := path.Join(getXRPLKeyDir(homePath, chainName)...) | ||
| keyRecords, err := LoadKeyRecord(keyRecordDir) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| kr, err := openXRPLKeyring(passphrase, homePath, chainName) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| signers := make(map[string]wallet.Signer) | ||
| for name, record := range keyRecords { | ||
| var signer wallet.Signer | ||
| switch record.Type { | ||
| case LocalSignerType: | ||
| secret, err := getXRPLSecret(kr, chainName, name) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| var w xrplwallet.Wallet | ||
| var wptr *xrplwallet.Wallet | ||
| switch record.SaveMethod { | ||
| case SaveMethodMnemonic: | ||
| wptr, err = xrplwallet.FromMnemonic(secret) | ||
| w = *wptr | ||
| case SaveMethodSeed: | ||
| w, err = xrplwallet.FromSecret(secret) | ||
| default: | ||
| return nil, fmt.Errorf("unsupported save method %s for key %s", record.SaveMethod, name) | ||
| } | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| signer = NewLocalSigner(name, w) | ||
| case RemoteSignerType: | ||
| if record.Address == "" { | ||
| return nil, fmt.Errorf("missing address for key %s", name) | ||
| } | ||
| if !addresscodec.IsValidClassicAddress(record.Address) { | ||
| return nil, fmt.Errorf("invalid address: %s", record.Address) | ||
| } | ||
|
|
||
| signer = NewRemoteSigner(name, record.Address, record.Url, record.Key) | ||
| default: | ||
| return nil, fmt.Errorf( | ||
| "unsupported signer type %s for chain %s, key %s", | ||
| record.Type, | ||
| chainName, | ||
| name, | ||
| ) | ||
| } | ||
|
|
||
| signers[name] = signer | ||
| } | ||
|
|
||
| return &XRPLWallet{ | ||
| Passphrase: passphrase, | ||
| Signers: signers, | ||
| HomePath: homePath, | ||
| ChainName: chainName, | ||
| }, nil | ||
| } | ||
|
|
||
| // SaveBySecret stores the secret in keyring and writes its record. | ||
| func (w *XRPLWallet) SaveBySecret(name string, secret string) (addr string, err error) { | ||
| if _, ok := w.Signers[name]; ok { | ||
| return "", fmt.Errorf("key name exists: %s", name) | ||
| } | ||
|
|
||
| privWallet, err := xrplwallet.FromSecret(secret) | ||
| if err != nil { | ||
| return | ||
| } | ||
|
|
||
| addr = privWallet.ClassicAddress.String() | ||
|
|
||
| if w.IsAddressExist(addr) { | ||
| return "", fmt.Errorf("address exists: %s", addr) | ||
| } | ||
|
|
||
| kr, err := openXRPLKeyring(w.Passphrase, w.HomePath, w.ChainName) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| if err := setXRPLSecret(kr, w.ChainName, name, secret); err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| record := NewKeyRecord(LocalSignerType, "", "", nil, SaveMethodSeed) | ||
| if err := w.saveKeyRecord(name, record); err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| return addr, nil | ||
| } | ||
|
|
||
| // SaveByMnemonic stores the mnemonic in keyring and writes its record. | ||
| func (w *XRPLWallet) SaveByMnemonic( | ||
| name string, | ||
| mnemonic string, | ||
| coinType uint32, | ||
| account uint, | ||
| index uint, | ||
| ) (addr string, err error) { | ||
| if _, ok := w.Signers[name]; ok { | ||
| return "", fmt.Errorf("key name exists: %s", name) | ||
| } | ||
| if coinType != xrplDefaultCoinType || account != 0 || index != 0 { | ||
| return "", fmt.Errorf("xrpl mnemonic derivation only supports m/44'/144'/0'/0/0") | ||
| } | ||
| if mnemonic == "" { | ||
| return "", fmt.Errorf("mnemonic is empty") | ||
| } | ||
|
|
||
| mnWallet, err := xrplwallet.FromMnemonic(mnemonic) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| addr = mnWallet.ClassicAddress.String() | ||
|
|
||
| if w.IsAddressExist(addr) { | ||
| return "", fmt.Errorf("address exists: %s", addr) | ||
| } | ||
|
|
||
| kr, err := openXRPLKeyring(w.Passphrase, w.HomePath, w.ChainName) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| if err := setXRPLSecret(kr, w.ChainName, name, mnemonic); err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| record := NewKeyRecord(LocalSignerType, "", "", nil, SaveMethodMnemonic) | ||
| if err := w.saveKeyRecord(name, record); err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| return addr, nil | ||
| } | ||
|
|
||
| // SaveRemoteSignerKey registers a remote signer under the given name. | ||
| func (w *XRPLWallet) SaveRemoteSignerKey(name, address, url string, key *string) error { | ||
| if _, ok := w.Signers[name]; ok { | ||
| return fmt.Errorf("key name exists: %s", name) | ||
| } | ||
|
|
||
| if !addresscodec.IsValidClassicAddress(address) { | ||
| return fmt.Errorf("invalid address: %s", address) | ||
| } | ||
|
|
||
| if w.IsAddressExist(address) { | ||
| return fmt.Errorf("address exists: %s", address) | ||
| } | ||
|
|
||
| record := NewKeyRecord(RemoteSignerType, address, url, key, "") | ||
| if err := w.saveKeyRecord(name, record); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // DeleteKey removes the signer named name, deleting its record. | ||
| func (w *XRPLWallet) DeleteKey(name string) error { | ||
| if _, ok := w.Signers[name]; !ok { | ||
| return fmt.Errorf("key name does not exist: %s", name) | ||
| } | ||
|
|
||
| if _, ok := w.Signers[name].(*LocalSigner); ok { | ||
| kr, err := openXRPLKeyring(w.Passphrase, w.HomePath, w.ChainName) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if err := deleteXRPLSecret(kr, w.ChainName, name); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| if err := w.deleteKeyRecord(name); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // GetSigners lists all signers. | ||
| func (w *XRPLWallet) GetSigners() []wallet.Signer { | ||
| signers := make([]wallet.Signer, 0, len(w.Signers)) | ||
| for _, signer := range w.Signers { | ||
| signers = append(signers, signer) | ||
| } | ||
|
|
||
| return signers | ||
| } | ||
|
|
||
| // GetSigner returns the signer with the given name and a flag indicating if it was found. | ||
| func (w *XRPLWallet) GetSigner(name string) (wallet.Signer, bool) { | ||
| signer, ok := w.Signers[name] | ||
| return signer, ok | ||
| } | ||
|
|
||
| // IsAddressExist returns true if the given address is already added. | ||
| func (w *XRPLWallet) IsAddressExist(address string) bool { | ||
| for _, signer := range w.Signers { | ||
| if signer.GetAddress() == address { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| // saveKeyRecord writes the KeyRecord to the file. | ||
| func (w *XRPLWallet) saveKeyRecord(name string, record KeyRecord) error { | ||
| b, err := toml.Marshal(record) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return os.Write(b, append(getXRPLKeyDir(w.HomePath, w.ChainName), fmt.Sprintf("%s.toml", name))) | ||
| } | ||
|
|
||
| // deleteKeyRecord deletes the KeyRecord file. | ||
| func (w *XRPLWallet) deleteKeyRecord(name string) error { | ||
| dir := path.Join(getXRPLKeyDir(w.HomePath, w.ChainName)...) | ||
| filePath := path.Join(dir, fmt.Sprintf("%s.toml", name)) | ||
| return os.DeletePath(filePath) | ||
| } |
There was a problem hiding this comment.
The XRPL wallet implementation lacks test coverage. The existing Geth wallet has comprehensive unit tests (wallet_test.go, local_signer_test.go, remote_signer_test.go, config_test.go, helper_test.go), but the XRPL wallet package has no tests. This is a significant gap in test coverage that should be addressed to ensure the reliability of the XRPL functionality.
relayer/tunnel_relayer.go
Outdated
| if t.lastRelayedSeq == nil || (t.lastRelayedSeq != nil && *t.lastRelayedSeq >= tunnelInfo.LatestSequence) { | ||
| t.Log.Debug("No new packet to relay", "sequence", *t.lastRelayedSeq) |
There was a problem hiding this comment.
When t.lastRelayedSeq is nil (first time), the log statement will cause a nil pointer dereference panic. The condition t.lastRelayedSeq == nil should return early before accessing *t.lastRelayedSeq in the log statement.
| for length != 0 && len(encoded) < length { | ||
| encoded += "0" | ||
| } |
There was a problem hiding this comment.
The padding logic in stringToHex adds zeros at the end, but hex padding should typically prepend zeros to maintain proper byte alignment. For example, encoding "A" as "41" padded to length 4 should be "0041", not "4100".
| package xrpl | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "math/big" | ||
| "time" | ||
|
|
||
| binarycodec "github.com/Peersyst/xrpl-go/binary-codec" | ||
| ledger "github.com/Peersyst/xrpl-go/xrpl/ledger-entry-types" | ||
| "github.com/Peersyst/xrpl-go/xrpl/transaction" | ||
| xrpltypes "github.com/Peersyst/xrpl-go/xrpl/transaction/types" | ||
| "github.com/shopspring/decimal" | ||
|
|
||
| "github.com/bandprotocol/falcon/internal/relayermetrics" | ||
| "github.com/bandprotocol/falcon/relayer/alert" | ||
| bandtypes "github.com/bandprotocol/falcon/relayer/band/types" | ||
| "github.com/bandprotocol/falcon/relayer/chains" | ||
| "github.com/bandprotocol/falcon/relayer/chains/types" | ||
| "github.com/bandprotocol/falcon/relayer/db" | ||
| "github.com/bandprotocol/falcon/relayer/logger" | ||
| "github.com/bandprotocol/falcon/relayer/wallet" | ||
| ) | ||
|
|
||
| var _ chains.ChainProvider = (*XRPLChainProvider)(nil) | ||
|
|
||
| // XRPLChainProvider handles interactions with XRPL. | ||
| type XRPLChainProvider struct { | ||
| Config *XRPLChainProviderConfig | ||
| ChainName string | ||
| // OracleAccount is derived from the XRPL wallet signers at runtime. | ||
| OracleAccount string | ||
|
|
||
| Client *Client | ||
|
|
||
| Log logger.Logger | ||
|
|
||
| Wallet wallet.Wallet | ||
| DB db.Database | ||
|
|
||
| Alert alert.Alert | ||
|
|
||
| FreeSigners chan wallet.Signer | ||
|
|
||
| nonceInterval time.Duration | ||
| } | ||
|
|
||
| // NewXRPLChainProvider creates a new XRPL chain provider. | ||
| func NewXRPLChainProvider( | ||
| chainName string, | ||
| client *Client, | ||
| cfg *XRPLChainProviderConfig, | ||
| log logger.Logger, | ||
| wallet wallet.Wallet, | ||
| alert alert.Alert, | ||
| ) (*XRPLChainProvider, error) { | ||
| if cfg.PriceScale == 0 { | ||
| cfg.PriceScale = 9 | ||
| } | ||
| if cfg.PriceScale > uint32(ledger.PriceDataScaleMax) { | ||
| return nil, fmt.Errorf( | ||
| "price_scale %d exceeds max %d", | ||
| cfg.PriceScale, | ||
| ledger.PriceDataScaleMax, | ||
| ) | ||
| } | ||
|
|
||
| return &XRPLChainProvider{ | ||
| Config: cfg, | ||
| ChainName: chainName, | ||
| Client: client, | ||
| Log: log.With("chain_name", chainName), | ||
| Wallet: wallet, | ||
| Alert: alert, | ||
| nonceInterval: time.Second, | ||
| }, nil | ||
| } | ||
|
|
||
| // Init connects to the XRPL chain. | ||
| func (cp *XRPLChainProvider) Init(ctx context.Context) error { | ||
| if err := cp.Client.Connect(ctx); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // SetDatabase assigns the given database instance. | ||
| func (cp *XRPLChainProvider) SetDatabase(database db.Database) { | ||
| cp.DB = database | ||
| } | ||
|
|
||
| // QueryTunnelInfo returns a best-effort tunnel info for XRPL. | ||
| func (cp *XRPLChainProvider) QueryTunnelInfo( | ||
| ctx context.Context, | ||
| tunnelID uint64, | ||
| tunnelDestinationAddr string, | ||
| ) (*types.Tunnel, error) { | ||
| tunnel := types.NewTunnel(tunnelID, tunnelDestinationAddr, true, 0, nil) | ||
| return tunnel, nil | ||
| } | ||
|
|
||
| // RelayPacket relays the packet to XRPL OracleSet transaction. | ||
| func (cp *XRPLChainProvider) RelayPacket(ctx context.Context, packet *bandtypes.Packet) error { | ||
| if cp.FreeSigners == nil { | ||
| return fmt.Errorf("signers not loaded") | ||
| } | ||
| signer := <-cp.FreeSigners | ||
| defer func() { | ||
| cp.FreeSigners <- signer | ||
| }() | ||
|
|
||
| log := cp.Log.With( | ||
| "tunnel_id", packet.TunnelID, | ||
| "sequence", packet.Sequence, | ||
| "signer_address", signer.GetAddress(), | ||
| ) | ||
|
|
||
| var lastErr error | ||
| var err error | ||
| sequence := uint64(0) | ||
| for retryCount := 1; retryCount <= cp.Config.MaxRetry; retryCount++ { | ||
| log.Info("Relaying a message", "retry_count", retryCount) | ||
|
|
||
| if sequence == 0 { | ||
| sequence, err = cp.Client.GetAccountSequenceNumber(ctx, signer.GetAddress()) | ||
| if err != nil { | ||
| log.Error("Get account sequence number error", "retry_count", retryCount, err) | ||
| lastErr = err | ||
| time.Sleep(cp.nonceInterval) | ||
| continue | ||
| } | ||
| } | ||
|
|
||
| tx, err := cp.buildOracleSetTx(packet, signer.GetAddress(), sequence) | ||
| if err != nil { | ||
| log.Error("Build OracleSet transaction error", "retry_count", retryCount, err) | ||
| lastErr = err | ||
| continue | ||
| } | ||
|
|
||
| if err := cp.Client.Autofill(&tx); err != nil { | ||
| log.Error("Autofill transaction error", "retry_count", retryCount, err) | ||
| lastErr = err | ||
| continue | ||
| } | ||
|
|
||
| encodedTx, err := binarycodec.Encode(tx) | ||
| if err != nil { | ||
| log.Error("Encode transaction error", "retry_count", retryCount, err) | ||
| lastErr = err | ||
| continue | ||
| } | ||
|
|
||
| txBlobBytes, err := signer.Sign([]byte(encodedTx)) | ||
| if err != nil { | ||
| log.Error("Sign transaction error", "retry_count", retryCount, err) | ||
| lastErr = err | ||
| continue | ||
| } | ||
|
|
||
| txHash, err := cp.Client.BroadcastTx(ctx, string(txBlobBytes)) | ||
| if err != nil { | ||
| log.Error("Broadcast transaction error", "retry_count", retryCount, err) | ||
| lastErr = err | ||
| continue | ||
| } | ||
|
|
||
| log.Info( | ||
| "Packet is successfully relayed", | ||
| "tx_hash", txHash, | ||
| "retry_count", retryCount, | ||
| ) | ||
|
|
||
| cp.saveRelayTx(packet, txHash) | ||
| relayermetrics.IncTxsCount(packet.TunnelID, cp.ChainName, types.TX_STATUS_SUCCESS.String()) | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| alert.HandleAlert( | ||
| cp.Alert, | ||
| alert.NewTopic(alert.RelayTxErrorMsg).WithTunnelID(packet.TunnelID).WithChainName(cp.ChainName), | ||
| lastErr.Error(), | ||
| ) | ||
| return fmt.Errorf("failed to relay packet after %d attempts", cp.Config.MaxRetry) | ||
| } | ||
|
|
||
| // QueryBalance queries balance by given key name from the destination chain. | ||
| func (cp *XRPLChainProvider) QueryBalance(ctx context.Context, keyName string) (*big.Int, error) { | ||
| signer, ok := cp.Wallet.GetSigner(keyName) | ||
| if !ok { | ||
| cp.Log.Error("Key name does not exist", "key_name", keyName) | ||
| return nil, fmt.Errorf("key name does not exist: %s", keyName) | ||
| } | ||
|
|
||
| return cp.Client.GetBalance(ctx, signer.GetAddress()) | ||
| } | ||
|
|
||
| // GetChainName retrieves the chain name from the chain provider. | ||
| func (cp *XRPLChainProvider) GetChainName() string { return cp.ChainName } | ||
|
|
||
| // ChainType retrieves the chain type from the chain provider. | ||
| func (cp *XRPLChainProvider) ChainType() types.ChainType { | ||
| return types.ChainTypeXRPL | ||
| } | ||
|
|
||
| // LoadSigners loads signers to prepare to relay the packet. | ||
| func (cp *XRPLChainProvider) LoadSigners() error { | ||
| cp.FreeSigners = chains.LoadSigners(cp.Wallet) | ||
| return nil | ||
| } | ||
|
|
||
| func (cp *XRPLChainProvider) buildOracleSetTx( | ||
| packet *bandtypes.Packet, | ||
| signerAddress string, | ||
| sequence uint64, | ||
| ) (transaction.FlatTransaction, error) { | ||
| providerHex, err := stringToHex("Band Protocol", 0) | ||
| if err != nil { | ||
| return transaction.FlatTransaction{}, err | ||
| } | ||
| dataClassHex, err := stringToHex("currency", 0) | ||
| if err != nil { | ||
| return transaction.FlatTransaction{}, err | ||
| } | ||
|
|
||
| priceDataSeries := make([]ledger.PriceDataWrapper, 0, len(packet.SignalPrices)) | ||
|
|
||
| for _, p := range packet.SignalPrices { | ||
| baseAsset, quoteAsset, err := parseAssetsFromSignal(p.SignalID) | ||
| if err != nil { | ||
| return transaction.FlatTransaction{}, err | ||
| } | ||
|
|
||
| priceDataSeries = append(priceDataSeries, ledger.PriceDataWrapper{ | ||
| PriceData: ledger.PriceData{ | ||
| BaseAsset: baseAsset, | ||
| QuoteAsset: quoteAsset, | ||
| AssetPrice: p.Price, | ||
| Scale: uint8(cp.Config.PriceScale), | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| tx := &transaction.OracleSet{ | ||
| BaseTx: transaction.BaseTx{ | ||
| Account: xrpltypes.Address(signerAddress), | ||
| TransactionType: transaction.OracleSetTx, | ||
| Sequence: uint32(sequence), | ||
| Fee: xrpltypes.XRPCurrencyAmount(12), | ||
| }, | ||
| OracleDocumentID: uint32(cp.Config.OracleID), | ||
| LastUpdatedTime: uint32(time.Now().Unix()), | ||
| Provider: providerHex, | ||
| AssetClass: dataClassHex, | ||
| PriceDataSeries: priceDataSeries, | ||
| } | ||
|
|
||
| return tx.Flatten(), nil | ||
| } | ||
|
|
||
| func (cp *XRPLChainProvider) saveRelayTx(packet *bandtypes.Packet, txHash string) { | ||
| signalPrices := make([]db.SignalPrice, 0, len(packet.SignalPrices)) | ||
| for _, p := range packet.SignalPrices { | ||
| signalPrices = append(signalPrices, *db.NewSignalPrice(p.SignalID, p.Price)) | ||
| } | ||
|
|
||
| tx := db.NewTransaction( | ||
| txHash, | ||
| packet.TunnelID, | ||
| packet.Sequence, | ||
| cp.ChainName, | ||
| types.ChainTypeXRPL, | ||
| cp.OracleAccount, | ||
| types.TX_STATUS_SUCCESS, | ||
| decimal.NullDecimal{}, | ||
| decimal.NullDecimal{}, | ||
| decimal.NullDecimal{}, | ||
| signalPrices, | ||
| nil, | ||
| ) | ||
|
|
||
| if cp.DB == nil { | ||
| return | ||
| } | ||
|
|
||
| if err := cp.DB.AddOrUpdateTransaction(tx); err != nil { | ||
| cp.Log.Error("Save transaction error", err) | ||
| alert.HandleAlert(cp.Alert, alert.NewTopic(alert.SaveDatabaseErrorMsg). | ||
| WithTunnelID(tx.TunnelID). | ||
| WithChainName(cp.ChainName), err.Error()) | ||
| } else { | ||
| alert.HandleReset(cp.Alert, alert.NewTopic(alert.SaveDatabaseErrorMsg). | ||
| WithTunnelID(tx.TunnelID). | ||
| WithChainName(cp.ChainName)) | ||
| } | ||
| } |
There was a problem hiding this comment.
The XRPL chain provider implementation lacks test coverage. While the EVM chain has tests in provider_test.go, keys_test.go, and other test files, the XRPL chain implementation has no corresponding tests. This is a significant gap in test coverage that should be addressed to ensure the reliability of the XRPL functionality.
|
|
||
| flagFile = "file" | ||
| flagPrivateKey = "private-key" | ||
| flagPrivateKey = "secret" |
There was a problem hiding this comment.
The flag name has changed from "private-key" to "secret" (line 11). This is a breaking change for CLI users who are using the --private-key flag. Consider maintaining backward compatibility by supporting both flag names, with the old one deprecated, or clearly document this breaking change in the changelog and migration guide.
| flagPrivateKey = "secret" | |
| flagPrivateKey = "private-key" // deprecated: prefer flagSecret | |
| flagSecret = "secret" |
| module github.com/bandprotocol/falcon | ||
|
|
||
| go 1.24.2 | ||
| go 1.24.3 |
There was a problem hiding this comment.
The Go version has been updated from 1.24.2 to 1.24.3. Note that Go 1.24 does not exist as of the knowledge cutoff date (January 2025). The latest stable Go version is 1.22.x. This appears to be a typo and should likely be "go 1.22.3" or a valid Go version.
| go 1.24.3 | |
| go 1.22.3 |
| if cpc.Fee == "" { | ||
| return fmt.Errorf("fee is required") |
There was a problem hiding this comment.
The Fee field is validated as required in the Validate method but is never used in the code. The hardcoded fee value of 12 drops is used instead (see provider.go line 251). Either remove the Fee field from the config and validation, or use the configured fee value instead of the hardcoded value.
relayer/wallet/xrpl/wallet.go
Outdated
| switch record.SaveMethod { | ||
| case SaveMethodMnemonic: | ||
| wptr, err = xrplwallet.FromMnemonic(secret) | ||
| w = *wptr |
There was a problem hiding this comment.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 45 out of 46 changed files in this pull request and generated 9 comments.
Comments suppressed due to low confidence (2)
relayer/chains/xrpl/provider.go:300
- The XRPL implementation lacks test coverage. While EVM chains have comprehensive tests (keys_test.go, provider_test.go, utils_test.go), XRPL has no test files. At minimum, tests should be added for critical functionality like wallet operations (SaveBySecret, SaveByMnemonic, DeleteKey), provider operations (RelayPacket, QueryBalance), and utility functions (stringToHex, parseAssetsFromSignal).
relayer/wallet/xrpl/wallet.go:270 - The XRPL wallet implementation lacks test coverage. Critical wallet operations like SaveBySecret, SaveByMnemonic, SaveRemoteSignerKey, and DeleteKey should have tests similar to the GethWallet test suite to ensure they work correctly and handle edge cases properly.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| unrelayedPackets := tunnelInfo.LatestSequence - targetContractInfo.LatestSequence | ||
| relayermetrics.SetUnrelayedPackets(t.TunnelID, unrelayedPackets) | ||
| // Specifically for XRPL, if it is the first time relaying (targetLatestSeq is nil) | ||
| // dont't set unwelayed packets metrics |
There was a problem hiding this comment.
Typo in comment: "unwelayed" should be "unrelayed".
| func (w *XRPLWallet) SaveBySecret(name string, secret string) (addr string, err error) { | ||
| if _, ok := w.Signers[name]; ok { | ||
| return "", fmt.Errorf("key name exists: %s", name) | ||
| } | ||
|
|
||
| privWallet, err := xrplwallet.FromSecret(secret) | ||
| if err != nil { | ||
| return | ||
| } | ||
|
|
||
| addr = privWallet.ClassicAddress.String() | ||
|
|
||
| if w.IsAddressExist(addr) { | ||
| return "", fmt.Errorf("address exists: %s", addr) | ||
| } | ||
|
|
||
| kr, err := openXRPLKeyring(w.Passphrase, w.HomePath, w.ChainName) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| if err := setXRPLSecret(kr, w.ChainName, name, secret); err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| record := NewKeyRecord(LocalSignerType, "", "", nil, SaveMethodSeed) | ||
| if err := w.saveKeyRecord(name, record); err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| return addr, nil | ||
| } |
There was a problem hiding this comment.
The SaveBySecret method doesn't update the in-memory Signers map after successfully saving the key. This means newly added keys won't be available in the current wallet instance until the wallet is reloaded via NewXRPLWallet. This is inconsistent with typical API expectations and could lead to bugs where callers expect the signer to be immediately available. The same issue exists in SaveByMnemonic, SaveRemoteSignerKey, and DeleteKey methods. Consider updating the Signers map immediately after the operations succeed.
| func (w *XRPLWallet) SaveByMnemonic( | ||
| name string, | ||
| mnemonic string, | ||
| coinType uint32, | ||
| account uint, | ||
| index uint, | ||
| ) (addr string, err error) { | ||
| if _, ok := w.Signers[name]; ok { | ||
| return "", fmt.Errorf("key name exists: %s", name) | ||
| } | ||
| if coinType != xrplDefaultCoinType || account != 0 || index != 0 { | ||
| return "", fmt.Errorf("xrpl mnemonic derivation only supports m/44'/144'/0'/0/0") | ||
| } | ||
| if mnemonic == "" { | ||
| return "", fmt.Errorf("mnemonic is empty") | ||
| } | ||
|
|
||
| mnWallet, err := xrplwallet.FromMnemonic(mnemonic) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| addr = mnWallet.ClassicAddress.String() | ||
|
|
||
| if w.IsAddressExist(addr) { | ||
| return "", fmt.Errorf("address exists: %s", addr) | ||
| } | ||
|
|
||
| kr, err := openXRPLKeyring(w.Passphrase, w.HomePath, w.ChainName) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| if err := setXRPLSecret(kr, w.ChainName, name, mnemonic); err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| record := NewKeyRecord(LocalSignerType, "", "", nil, SaveMethodMnemonic) | ||
| if err := w.saveKeyRecord(name, record); err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| return addr, nil | ||
| } |
There was a problem hiding this comment.
The SaveByMnemonic method doesn't update the in-memory Signers map after successfully saving the key, similar to SaveBySecret. The newly added key won't be available in the current wallet instance.
| "retry_count", retryCount, | ||
| ) | ||
|
|
||
| cp.saveRelayTx(packet, txHash) |
There was a problem hiding this comment.
The OracleAccount field is never initialized and remains empty. When saveRelayTx is called at line 176, it uses cp.OracleAccount (line 276) to store the transaction in the database, but this field is empty. The signer's address should be passed to saveRelayTx as a parameter (similar to how EVM provider handles it in createTx), or the OracleAccount should be set when LoadSigners is called.
| for length != 0 && len(encoded) < length { | ||
| encoded += "0" | ||
| } |
There was a problem hiding this comment.
The stringToHex function pads the hex string by appending '0' characters (line 15), but this may not be the intended behavior. If the goal is to pad the hex-encoded bytes to a specific length, appending '0' characters means adding '0' to the hex string, not zero bytes. For example, if you're trying to pad to 40 characters and have "48656C6C6F" (5 bytes = 10 hex chars for "Hello"), adding "0" characters gives "48656C6C6F000000..." which is correct. However, the logic should be clearer about whether this is intentional padding of the hex representation or if zero bytes should be added to the original string before encoding.
| unrelayedPackets := tunnelInfo.LatestSequence - targetContractInfo.LatestSequence | ||
| relayermetrics.SetUnrelayedPackets(t.TunnelID, unrelayedPackets) | ||
| // Specifically for XRPL, if it is the first time relaying (targetLatestSeq is nil) | ||
| // dont't set unwelayed packets metrics |
There was a problem hiding this comment.
Typo in comment: "dont't" should be "don't".
| func (w *XRPLWallet) SaveRemoteSignerKey(name, address, url string, key *string) error { | ||
| if _, ok := w.Signers[name]; ok { | ||
| return fmt.Errorf("key name exists: %s", name) | ||
| } | ||
|
|
||
| if !addresscodec.IsValidClassicAddress(address) { | ||
| return fmt.Errorf("invalid address: %s", address) | ||
| } | ||
|
|
||
| if w.IsAddressExist(address) { | ||
| return fmt.Errorf("address exists: %s", address) | ||
| } | ||
|
|
||
| record := NewKeyRecord(RemoteSignerType, address, url, key, "") | ||
| if err := w.saveKeyRecord(name, record); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
The SaveRemoteSignerKey method doesn't update the in-memory Signers map after successfully saving the remote signer. The newly registered remote signer won't be available in the current wallet instance.
| func (w *XRPLWallet) DeleteKey(name string) error { | ||
| if _, ok := w.Signers[name]; !ok { | ||
| return fmt.Errorf("key name does not exist: %s", name) | ||
| } | ||
|
|
||
| if _, ok := w.Signers[name].(*LocalSigner); ok { | ||
| kr, err := openXRPLKeyring(w.Passphrase, w.HomePath, w.ChainName) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if err := deleteXRPLSecret(kr, w.ChainName, name); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| if err := w.deleteKeyRecord(name); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
The DeleteKey method doesn't remove the signer from the in-memory Signers map after successfully deleting the key. The deleted signer will still be accessible via GetSigner until the wallet is reloaded.
| Account: xrpltypes.Address(signerAddress), | ||
| TransactionType: transaction.OracleSetTx, | ||
| Sequence: uint32(sequence), | ||
| Fee: xrpltypes.XRPCurrencyAmount(12), |
There was a problem hiding this comment.
The Fee field in XRPLChainProviderConfig is validated as required (line 44-46 in config.go) but is never actually used. The transaction fee is hardcoded to 12 drops at line 252. Either the Fee configuration should be used here (parsing it and using cp.Config.Fee), or the Fee field and its validation should be removed from the config.
Fixed: #XXXX
Implementation details
Please ensure the following requirements are met before submitting a pull request:
CHANGELOG_UNRELEASED.mdFiles changedtab in the Github PR explorer)