Skip to content
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

Add Sanction List #30

Merged
merged 25 commits into from
Feb 13, 2024
Merged

Add Sanction List #30

merged 25 commits into from
Feb 13, 2024

Conversation

ppoliani
Copy link
Contributor

@ppoliani ppoliani commented Jan 28, 2024

Add sanction list based to the discussion here #5.

The logic goes like this:

  1. Start a thread that periodically fetches the latest list of sanctioned BTC addresses from here
  2. If the list is fresh it will extract all BTC addresses from that XML file and store in-memory
  3. Exposes a is_sanctioned method that returns true if the address is in the sanction list.

@ppoliani
Copy link
Contributor Author

It takes around 35s to update fetch the latest OFAC list and extract all addresses from XML to the in memory hashmap. The logic runs in a separate non-blocking thread so it should not cause any issues. Also, they update the list every few days so it's gonna run that often

@ppoliani
Copy link
Contributor Author

ppoliani commented Jan 30, 2024

I believe we should start this thread when we run the orchestrator. It should wait for the fist sync of the OFAC list before it starts

src/address_verifier.rs Outdated Show resolved Hide resolved
src/address_verifier.rs Outdated Show resolved Hide resolved
src/address_verifier.rs Outdated Show resolved Hide resolved
src/address_verifier.rs Outdated Show resolved Hide resolved
src/address_verifier.rs Outdated Show resolved Hide resolved
@ppoliani ppoliani changed the title [WIP] Add Sanction List Add Sanction List Jan 31, 2024
@mimoo
Copy link
Contributor

mimoo commented Feb 1, 2024

BTW let me know when this is ready for review (looks like CI doesn't pass atm)

@ppoliani
Copy link
Contributor Author

ppoliani commented Feb 1, 2024

BTW let me know when this is ready for review (looks like CI doesn't pass atm)

Fixed the lint issues. You can review this :)

Copy link
Contributor

@mimoo mimoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is amazing :) thanks so much again for these PRs!

One thing that'd be great to have is a test just to easily see what the sanction list is, how big it is, and how much time it takes to sync

src/bob_request.rs Show resolved Hide resolved
}
}

/// Handles bob request from A to Z.
pub async fn handle_request(&self, bob_request: &BobRequest) -> Result<BobResponse> {
// Validate transaction before forwarding it, and get smart contract
bob_request
.check_compliance(Arc::clone(&self.compliance))
.await?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wondering: should individual nodes perform that check as well? What would be the rational for not performing that check at the node level (and would node be liable if something bad happens there...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could, but we've got orchestrator as a centralized entity that coordinates the entire process.

src/committee/orchestrator.rs Show resolved Hide resolved
src/compliance.rs Show resolved Hide resolved
}

impl Compliance {
const BTC_ID: &'static str = "344";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you document where you got that value from? (if it's possible)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I couldn't find any documentation. This is a gov document, didn't expect anything better to be honest. I had to look into the XML file and find that number.

src/compliance.rs Show resolved Hide resolved
let last_update = Arc::clone(&self.last_update);

spawn(async move {
let mut interval = interval(Duration::from_secs(600));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should hardcode that value somewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes I will do that

);

// Sync sanction list in a parallel thread
compliance.start();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't you have to use the joinhandle returned?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not if you don't want to wait for it to complete

@mimoo mimoo merged commit 575b135 into sigma0-dev:main Feb 13, 2024
1 check passed
@ppoliani
Copy link
Contributor Author

Sanction ist

I've added a comment regarding this.

#30 (comment)

It's an XML file and is 92MB big.

@ppoliani ppoliani deleted the feat/sanctioned_list branch February 13, 2024 08:24
@ppoliani ppoliani mentioned this pull request Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants