search

kadena-io / marmalade

Decentralized Infrastructure for Poly-Fungibles and NFTs
24 stars 16 forks source link

V2: The timeout argument of `(sale` should be a `time` #131

Closed CryptoPascal31 closed 1 year ago

CryptoPascal31 commented 1 year ago

Context (Environment)

Since this PR: https://github.com/kadena-io/marmalade/pull/119, the timeout argument has been transformed from a time to an Unix timestamp encoded as a decimal.

Pact has an high level time type to manage conveniently and safely datetimes. Moreover it seems that Kadena.js handles this time very conveniently by mapping automatically JS native date types to Pact, avoiding possible bugs and errors.

Why use an UNIX timestamp ? What a strange idea...

The mentioned PR gives no rationale. And after reading the code, I don't find any good reason to do that. Is it possible to explain, please ?

Detailed Description

I propose to revert back the timeout to the native Pact time.

Possible Implementation

No response

ggobugi27 commented 1 year ago

We've changed timeout to decimal type with the addition of offers without timeout. When timeout=0.0 offer does not have a timeout and the seller can sign to withdraw the offer at any time. We've added the to-timestamp function that computes a time argument into timestamp which allows users to pass in timeout like before.

CryptoPascal31 commented 1 year ago

Sorry I don't understand the relation between the feature "offers without timeout" and "Unix style timestamps"

Can you elaborate ?

ggobugi27 commented 1 year ago

Sorry if my point wasn't clear. When adding optional timeout, we thought it was more intuitive to use 0.0 instead of setting an EPOCH as time format for comparison.

CryptoPascal31 commented 1 year ago

why not just simply create a constant like:

(defconst NO-TIMEOUT:time (time "0000-01-01T00:00:00Z"))

(any other arbitrary timedate before genesis is good as well)

and use that constant to handle the "no timeout" cases through the code.

It would avoid to break the API and add another layer of non-useful complexity.

jermaine150 commented 1 year ago

Closing this, changes have been made in: https://github.com/kadena-io/marmalade/pull/141

The integer type represents "unix timestamp" in v2, instead of "block numbers" in v1, an API upgrade for user convenience. Some rationals for using the unix timestamp :