(feat) Add OFAC list check #236
Conversation
WalkthroughThe changes introduce an Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ChainClient
participant OfacChecker
User->>ChainClient: Initialize with address
ChainClient->>OfacChecker: Check if address is blacklisted
OfacChecker-->>ChainClient: Return blacklist status
ChainClient-->>User: Proceed or return error
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (5)
Files skipped from review as they are similar to previous changes (4)
Additional comments not posted (1)
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
PavelInjective
force-pushed
the
ofac-list-
branch
3 times, most recently
from
28d34ce
to
19000bd
Compare
| @@ -32,7 +32,7 @@ func LoadTxFromFile(fileName string) ([]byte, error) { | |||
| } | |||
|
|
|||
| func main() { | |||
| network := common.LoadNetwork("devnet", "") | |||
| network := common.LoadNetwork("devnet", "lb") | |||
There was a problem hiding this comment.
It is not necessary to provide the node name for devnet network. That parameter is only used for testnet and mainnet.
There was a problem hiding this comment.
actually, it should be testnet. let me update the PR
There was a problem hiding this comment.
sorry, this is example file. it should not have been altered at all. force-pushing the correct version of the commit.
client/chain/chain.go
Outdated
| func (cc *chainClient) loadOfacList() error { | ||
| response, err := http.Get(defaultOfacListURL) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer response.Body.Close() | ||
|
|
||
| if response.StatusCode != http.StatusOK { | ||
| return fmt.Errorf("request to the OFAC upstream failed with code: %s", response.Status) | ||
| } | ||
|
|
||
| body, err := io.ReadAll(response.Body) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| var ofacList []string | ||
| if err := json.Unmarshal(body, &ofacList); err != nil { | ||
| return err | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
As per in the Python SDK, I think the idea is to keep a local copy of the ofac.json file. The helper method to download the file will be usefull (and I think required), but it should be in a separate module.
The ChainClient should just load the addresses from the local file
client/chain/chain.go
Outdated
| isAddrInTheList := func(list []string, addr string) bool { | ||
| for _, item := range list { | ||
| if item == addr { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
| k, err := txf.Keybase().Key(clientCtx.FromName) | ||
| if err != nil { | ||
| err = errors.Wrap(err, "error parsing signer account address") | ||
| return nil, err | ||
| } | ||
| signerAddressPubKey, err := k.GetPubKey() | ||
| if err != nil { | ||
| err = errors.Wrap(err, "error getting signer public key") | ||
| return nil, err | ||
| } | ||
| if isAddrInTheList(c.ofacTxList, sdk.AccAddress(signerAddressPubKey.Address()).String()) { | ||
| err = errors.Errorf("Address is in the OFAC list") | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Can we extract this to a method that does the validation and returns an error if the validation fails?
client/chain/chain.go
Outdated
| isAddrInTheList := func(list []string, addr string) bool { | ||
| for _, item := range list { | ||
| if item == addr { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
Is this anonymous function required?
There was a problem hiding this comment.
not really. we use it only once. i thought it would be used twice, that's why I created a function.
client/chain/chain_test.go
Outdated
| func TestOfacList(t *testing.T) { | ||
| network := common.LoadNetwork("testnet", "lb") | ||
| tmClient, err := rpchttp.New(network.TmEndpoint, "/websocket") | ||
| assert.NoError(t, err) | ||
|
|
||
| senderAddress, cosmosKeyring, err := accountForTests() | ||
| assert.NoError(t, err) | ||
|
|
||
| clientCtx, err := NewClientContext( | ||
| network.ChainId, | ||
| senderAddress.String(), | ||
| cosmosKeyring, | ||
| ) | ||
| assert.NoError(t, err) | ||
|
|
||
| clientCtx = clientCtx.WithNodeURI(network.TmEndpoint).WithClient(tmClient) | ||
|
|
||
| exchangeClient, err := exchangeclient.NewExchangeClient(network) | ||
| assert.NoError(t, err) | ||
| ctx := context.Background() | ||
| req := spotExchangePB.MarketsRequest{ | ||
| MarketStatus: "active", | ||
| } | ||
| res, err := exchangeClient.GetSpotMarkets(ctx, &req) | ||
| assert.NoError(t, err) | ||
|
|
||
| marketsAssistant, err := NewMarketsAssistantInitializedFromChain(ctx, exchangeClient) | ||
| assert.NoError(t, err) | ||
|
|
||
| cc, err := createClient(senderAddress, cosmosKeyring, network) | ||
| assert.NoError(t, err) | ||
|
|
||
| defaultSubaccountID := cc.DefaultSubaccount(senderAddress) | ||
| marketId := res.Markets[0].MarketId | ||
| amount := decimal.NewFromFloat(2) | ||
| price := decimal.NewFromFloat(1.02) | ||
|
|
||
| order := cc.CreateSpotOrder( | ||
| defaultSubaccountID, | ||
| &SpotOrderData{ | ||
| OrderType: exchangetypes.OrderType_BUY, //BUY SELL BUY_PO SELL_PO | ||
| Quantity: amount, | ||
| Price: price, | ||
| FeeRecipient: senderAddress.String(), | ||
| MarketId: marketId, | ||
| }, | ||
| marketsAssistant, | ||
| ) | ||
|
|
||
| msg := new(exchangetypes.MsgCreateSpotLimitOrder) | ||
| msg.Sender = senderAddress.String() | ||
| msg.Order = exchangetypes.SpotOrder(*order) | ||
|
|
||
| accNum, accSeq := cc.GetAccNonce() | ||
|
|
||
| cc.(*chainClient).ofacTxList = []string{ | ||
| senderAddress.String(), | ||
| } | ||
| _, err = cc.BuildSignedTx(clientCtx, accNum, accSeq, 20000, msg) | ||
| assert.Error(t, err) | ||
| } |
There was a problem hiding this comment.
We should create a test that does not connect to a any node or indexer if possible. In this case we should not to create an ExchangeClient instance to create a message to test the OFAC restriction
There was a problem hiding this comment.
Great first PR @PavelInjective.
Please lets use dev as the base branch.
Besides the comments I have added, we need to add the validation also in the ChainClient methods that return instances of authztypes.MsgGrant. In that case the address to check is the granter address
PavelInjective
force-pushed
the
ofac-list-
branch
7 times, most recently
from
2213553
to
7262c16
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
client/chain/ofac.go (1)
46-72: LGTM, but consider improving error handling.The
DownloadOfacListfunction is correctly implemented and handles the download and saving of the OFAC list well. It checks if the download was successful by verifying the HTTP status code, creates the OFAC list file, and writes the downloaded data to it.However, consider the following improvements:
- Handle more error cases, such as network errors or file system errors, to make the function more robust.
- Provide more detailed error messages to help with debugging in case something goes wrong.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- client/chain/chain.go (4 hunks)
- client/chain/chain_test.go (3 hunks)
- client/chain/ofac.go (1 hunks)
- examples/chain/ofac/1_DownloadOfacList/example.go (1 hunks)
- ofac.json (1 hunks)
Additional comments not posted (9)
examples/chain/ofac/1_DownloadOfacList/example.go (1)
1-12: LGTM!The example program is correctly implemented and serves as a good demonstration of how to download the OFAC list using the
chainclient.DownloadOfacListfunction.client/chain/ofac.go (3)
23-36: LGTM!The
NewOfacCheckerfunction is correctly implemented and handles the initialization of theOfacCheckerwell. It checks if the OFAC list file exists, downloads it if it doesn't, and loads the OFAC list into memory.
38-44: LGTM!The
getOfacListPathfunction is correctly implemented and provides a reliable way to locate the OFAC list file by searching for the "sdk-go" directory and returning the path to the OFAC list file relative to it.
74-85: LGTM!The
loadOfacListmethod is correctly implemented and handles the loading of the OFAC list into memory well. It reads the OFAC list file and unmarshals the JSON data into theofacListfield ofOfacChecker.ofac.json (1)
1-48: LGTM!The
ofac.jsonfile is a simple JSON array of blacklisted addresses and does not require any changes. It serves as a data source for theOfacChecker.client/chain/chain_test.go (1)
58-97: LGTM!The
TestOfacListfunction is a well-structured test that covers the key aspects of OFAC list handling. It sets up the necessary prerequisites, performs the test by attempting to create a new chain client with a blacklisted address, asserts the expected error, and cleans up after the test.The test function is approved.
client/chain/chain.go (3)
324-324: Looks good!The addition of the
ofacCheckerfield to thechainClientstruct enhances its capability to check if an address is blacklisted according to OFAC regulations.The struct field addition is approved.
445-460: Great job integrating OFAC list checks!The modifications to the
NewChainClientfunction enhance the security and compliance aspects of thechainClientby integrating checks against the OFAC list. It prevents the creation of a new chain client if the account's address is blacklisted.The function modifications are approved.
1200-1202: Consider returning an error instead of panicking.The modification to the
BuildExchangeAuthzfunction introduces a check if the granter's address is blacklisted, which enhances security by preventing the creation of exchange authorization for blacklisted addresses.However, similar to the
BuildGenericAuthzfunction, the use of panic for error handling is not ideal. It abruptly terminates the program and does not provide a graceful way to handle the error.Consider returning an error instead of panicking:
- if c.ofacChecker.IsBlacklisted(granter) { - panic("Address is in the OFAC list") - } + if c.ofacChecker.IsBlacklisted(granter) { + return nil, errors.New("granter address is in the OFAC list") + }This way, the caller of the function can handle the error appropriately.
client/chain/ofac.go
Outdated
| func (oc *OfacChecker) IsBlacklisted(address string) bool { | ||
| for _, item := range oc.ofacList { | ||
| if item == address { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
LGTM, but consider using a more efficient data structure.
The IsBlacklisted method is correctly implemented and provides a simple way to check if an address is blacklisted by iterating over the ofacList and returning true if the address is found in the list.
However, consider using a more efficient data structure, such as a hash set, for faster lookups, especially if the OFAC list grows large.
To use a hash set, modify the ofacList field to be a map[string]bool and update the loadOfacList method to populate the map:
type OfacChecker struct {
ofacListPath string
ofacList map[string]bool
}
func (oc *OfacChecker) loadOfacList() error {
// ...
oc.ofacList = make(map[string]bool)
for _, address := range list {
oc.ofacList[address] = true
}
return nil
}Then, update the IsBlacklisted method to use the map:
func (oc *OfacChecker) IsBlacklisted(address string) bool {
return oc.ofacList[address]
}There was a problem hiding this comment.
@PavelInjective we should consider this recommendation from CodeRabbit
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
okay, let's implement this as a map. however, iterating over a thousand of addresses when sdk instance is created is not a big deal.
client/chain/chain_test.go
Outdated
| err = DownloadOfacList() | ||
| assert.NoError(t, err) |
There was a problem hiding this comment.
Let's have a test here similar to the Python SDK test, that does not require downloading the real file (a test that will not fail if executed in an environment without internet connection)
client/chain/ofac.go
Outdated
| func (oc *OfacChecker) IsBlacklisted(address string) bool { | ||
| for _, item := range oc.ofacList { | ||
| if item == address { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
@PavelInjective we should consider this recommendation from CodeRabbit
PavelInjective
force-pushed
the
ofac-list-
branch
from
7262c16
to
e5a8990
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- client/chain/chain.go (4 hunks)
- client/chain/chain_test.go (3 hunks)
- client/chain/ofac.go (1 hunks)
- client/metadata/ofac.json (1 hunks)
- examples/chain/ofac/1_DownloadOfacList/example.go (1 hunks)
Files skipped from review due to trivial changes (1)
- examples/chain/ofac/1_DownloadOfacList/example.go
Files skipped from review as they are similar to previous changes (2)
- client/chain/chain_test.go
- client/chain/ofac.go
Additional comments not posted (4)
client/metadata/ofac.json (1)
1-48: LGTM! The file structure and content are valid.The file is a valid JSON array containing Ethereum addresses, which aligns with the expected OFAC blacklist format based on the file name and PR context.
client/chain/chain.go (3)
324-324: LGTM!The addition of the
ofacCheckerfield to thechainClientstruct is approved.
447-450: LGTM!The changes to the
NewChainClientfunction are approved:
- Initializing the
ofacCheckeris necessary to enable the OFAC list check functionality.- Checking if the account's address is blacklisted during client initialization enhances security by preventing the creation of a client for a blacklisted address.
Also applies to: 458-460
1166-1168: Consider returning an error instead of panicking.The modification to the
BuildGenericAuthzfunction introduces a check if the granter's address is blacklisted, which enhances security by preventing the creation of generic authorization for blacklisted addresses.However, the use of panic for error handling is not ideal. It abruptly terminates the program and does not provide a graceful way to handle the error.
Consider returning an error instead of panicking:
- if c.ofacChecker.IsBlacklisted(granter) { - panic("Address is in the OFAC list") - } + if c.ofacChecker.IsBlacklisted(granter) { + return nil, errors.New("granter address is in the OFAC list") + }This way, the caller of the function can handle the error appropriately.
Likely invalid or redundant comment.
PavelInjective
force-pushed
the
ofac-list-
branch
from
e5a8990
to
a091b65
Compare
PavelInjective
force-pushed
the
ofac-list-
branch
from
a091b65
to
b529743
Compare
PavelInjective
force-pushed
the
ofac-list-
branch
from
b529743
to
a68a1a3
Compare
PavelInjective
force-pushed
the
ofac-list-
branch
from
a68a1a3
to
fedf1a8
Compare
client/chain/chain.go
Outdated
| @@ -440,15 +442,23 @@ func NewChainClient( | |||
| subaccountToNonce: make(map[ethcommon.Hash]uint32), | |||
| } | |||
|
|
|||
| _ = NewTxFactory(ctx).WithSequence(0).WithAccountNumber(0).WithGas(0) | |||
There was a problem hiding this comment.
@PavelInjective what is this line for?
Summary by CodeRabbit
New Features
OfacCheckerto manage and verify addresses against the OFAC blacklist.Bug Fixes
Tests
Documentation