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

feat: add horizon types to tap_graph #270

Merged
merged 1 commit into from
Jan 29, 2025
Merged

Conversation

gusinacio
Copy link
Contributor

@gusinacio gusinacio commented Jan 28, 2025

For now, handling both receipts is delegated to the lib user but serialization might be added in a future PR.

@gusinacio gusinacio force-pushed the gustavo/tap-graph-v2 branch from 19f140c to f323ed7 Compare January 28, 2025 22:29
@gusinacio gusinacio force-pushed the gustavo/tap-graph-v2 branch from f323ed7 to b7bd0da Compare January 28, 2025 22:30
@coveralls
Copy link

coveralls commented Jan 28, 2025

Pull Request Test Coverage Report for Build 13020749107

Details

  • 30 of 98 (30.61%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-3.5%) to 77.348%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tap_graph/src/v2/receipt.rs 30 36 83.33%
tap_graph/src/v2/rav.rs 0 62 0.0%
Totals Coverage Status
Change from base Build 13017980916: -3.5%
Covered Lines: 1079
Relevant Lines: 1395

💛 - Coveralls

@gusinacio gusinacio marked this pull request as ready for review January 29, 2025 14:45
Comment on lines +25 to +47
sol! {
/// Holds information needed for promise of payment signed with ECDSA
///
/// We use camelCase for field names to match the Ethereum ABI encoding
#[derive(Debug, Serialize, Deserialize, Eq, PartialEq)]
struct ReceiptAggregateVoucher {
/// Unique allocation id this RAV belongs to
address allocationId;
// The address of the payer the RAV was issued by
address payer;
// The address of the data service the RAV was issued to
address dataService;
// The address of the service provider the RAV was issued to
address serviceProvider;
// The RAV timestamp, indicating the latest TAP Receipt in the RAV
uint64 timestampNs;
// Total amount owed to the service provider since the beginning of the
// payer-service provider relationship, including all debt that is already paid for.
uint128 valueAggregate;
// Arbitrary metadata to extend functionality if a data service requires it
bytes metadata;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@gusinacio could you just explain why this needs to be abi encoded, presumably because this is interacting with a contract. Would be great to be able to make the link here. Can also do it in a subsequent PR if you wanna explain here quickly 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definition of encodeType.
https://eips.ethereum.org/EIPS/eip-712#definition-of-domainseparator
https://docs.rs/alloy/latest/alloy/sol_types/macro.sol.html

sol macro generates the encodeType for EIP712, it doesn't use serde and at the time, there was no way to "rename camelCase" like we do in serde

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it looks like it still doesn't have that alloy-rs/core#570

receipts: &[Eip712SignedMessage<Receipt>],
previous_rav: Option<Eip712SignedMessage<Self>>,
) -> Result<Self, AggregationError> {
//TODO(#29): When receipts in flight struct in created check that the state
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
//TODO(#29): When receipts in flight struct in created check that the state
//TODO(#29): When receipts in flight struct in created check that the state

I don't understand what this is trying to say, can we make the TODO clearer, please! 🙏

fn get_current_timestamp_u64_ns() -> Result<u64, SystemTimeError> {
Ok(SystemTime::now().duration_since(UNIX_EPOCH)?.as_nanos() as u64)
}
impl Receipt {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
impl Receipt {
impl Receipt {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe you could add a clippy lint rule in a following PR so it formats automatically like you are suggesting.

// Check that nonces are different
// Note: This test has an *extremely low* (~1/2^64) probability of false failure, if a failure happens
// once it is not neccessarily a sign of an issue. If this test fails more than once, especially
// in a short period of time (within a ) then there may be an issue with randomness
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// in a short period of time (within a ) then there may be an issue with randomness
// in a short period of time (within a ) then there may be an issue with randomness

"within a "...?

@gusinacio gusinacio merged commit b8e424e into main Jan 29, 2025
8 checks passed
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.

3 participants