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

[fortuna] Add tests #1133

Merged
merged 7 commits into from
Nov 20, 2023
Merged

[fortuna] Add tests #1133

merged 7 commits into from
Nov 20, 2023

Conversation

jayantk
Copy link
Contributor

@jayantk jayantk commented Nov 6, 2023

Add some tests for the revelations endpoint. I also factored out a trait that lets us mock the on-chain contract for testing purposes, which I'm using for these tests.

Some of this code is definitely well beyond my level of real rust knowledge, so please let me know if there are better ways to do any of the things I'm doing here.

Copy link

vercel bot commented Nov 6, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
example-oracle-amm ⬜️ Ignored (Inspect) Visit Preview Nov 20, 2023 7:34pm
xc-admin-frontend ⬜️ Ignored (Inspect) Visit Preview Nov 20, 2023 7:34pm

impl ApiState {
pub fn new(chains: &[(ChainId, BlockchainState)]) -> ApiState {
let map: HashMap<ChainId, BlockchainState> =
chains.into_iter().map(|r| (*r).clone()).collect();
Copy link
Contributor

Choose a reason for hiding this comment

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

.map(|r| (*r).clone() can be replaced with just .cloned()

};

const PROVIDER: Address = Address::zero();
const OTHER_PROVIDER: Lazy<Address> = Lazy::new(|| Address::from_low_u64_be(1));
Copy link
Contributor

@Reisen Reisen Nov 7, 2023

Choose a reason for hiding this comment

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

I generally think lazy_static is a better library/interface for this, it's sad ethers provides no const initializers from arrays. Though it seems this type is just an alias for H160 and the content of that is public, so you might be able to do:

const OTHER_PROVIDER: Address = Address([1u8; 20]);


const PROVIDER: Address = Address::zero();
const OTHER_PROVIDER: Lazy<Address> = Lazy::new(|| Address::from_low_u64_be(1));
const ETH_CHAIN: Lazy<Arc<HashChainState>> = Lazy::new(|| {
Copy link
Contributor

Choose a reason for hiding this comment

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

The problem with having these as Arc's is their state will persist between other tests. In general I think It's a bad idea to do this, especially as Rust runs tests asynchronously leading to non-deterministic and unpredictable behaviour. I think you're better off moving these inside your test_server() initializer and returning them there, or just setting them up at the start of the test.

#[async_trait]
pub trait EntropyReader: Send + Sync {
/// Get an in-flight request (if it exists)
/// Note that if we support additional blockchains in the future, the type of `provider` may
Copy link
Contributor

@Reisen Reisen Nov 7, 2023

Choose a reason for hiding this comment

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

I think you might want to deal with the generic here now, because this trait is a public interface. If we add a new chain this will break peoples working code and if It's not generic it defeats the purpose of the trait in the first place. You can get here quite fast with a less likely to break trait using associated types:

pub trait EntropyReader: Send + Sync {
    type Provider;
    async fn get_request(&self, provider: Self::Provider, sequence_number: u64)
        -> Result<Option<Request>>;
}

Then in your ethereum.rs you can do:

impl EntropyReader for PythContract {
    type Provider = Address;

To associate the Ethereum type for that implementation.

@jayantk jayantk merged commit a22b202 into main Nov 20, 2023
5 checks passed
@jayantk jayantk deleted the random19 branch November 20, 2023 20:44
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