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

Send offer duration from ASB to CLI #1662

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ikmckenz
Copy link
Contributor

Eventually I like the idea of this being a "max offer duration", which could be configurable, and then advanced users could have their own variable duration based around things like volatility. But for now I think this accomplishes the goals of #1644 .

@binarybaron
Copy link
Collaborator

Yes, that's exactly what I imagined! Making the duration variable can be added at a later point.

But unfortunately this is not backwards compatible is it? If it's not possible without introducing a seperate new libp2p protocol I'd suggest we postpone mergin this this until we need to introduce another (more important) breaking network protocol change. What do do you think?

@delta1
Copy link
Collaborator

delta1 commented May 31, 2024

It can probably be made optional and remain backwards compatible?

@ikmckenz
Copy link
Contributor Author

ikmckenz commented Jun 3, 2024

Perhaps #1268 should be implemented prior to merging

@ikmckenz ikmckenz force-pushed the transmit-offer-valid-duration branch from 0bf96a8 to 2106bf8 Compare June 29, 2024 20:44
On the ASB side this is max_swap_timeout, and may eventually be
configurable, or dynamic. On the CLI side this is just received as
valid_duration, an Optional Duration.

Signed-off-by: Ian McKenzie <sirreal.ian@gmail.com>
@ikmckenz ikmckenz force-pushed the transmit-offer-valid-duration branch from 2106bf8 to c600405 Compare June 29, 2024 20:47
@ikmckenz
Copy link
Contributor Author

ikmckenz commented Jun 29, 2024

Made the valid_duration (the BidQuote part the CLI receives) Optional. Not sure if the the integration tests cover this kind of change that may or may not be backwards compatible. Perhaps additional integration test can be made where one party uses the current code, and the other party pulls the most recent released code from GitHub and runs that? Not sure.

@ikmckenz ikmckenz marked this pull request as ready for review June 29, 2024 20:58
@binarybaron
Copy link
Collaborator

Have you tested manually if this works?

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