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 CSV format for trades #1540
Conversation
* @param refData the reference data | ||
* @return the loaded trades, all errors are captured in the result | ||
*/ | ||
static SwapTrade parse(CsvRow row, TradeInfo info, ReferenceData refData) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused refData arg
while (payReceive.isPresent()) { | ||
legs.add(parseLeg(row, "Leg " + i + " ")); | ||
i++; | ||
payReceive = row.findValue("Leg " + i + " " + DIRECTION_FIELD); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could use private findValue() here for consistency
.map(s -> new Integer(s)); | ||
HolidayCalendarId cal = findValue(row, leg, daysCalField) | ||
.map(s -> HolidayCalendarId.of(s)) | ||
.orElse(HolidayCalendarIds.NO_HOLIDAYS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this default behaviour of using "NO_HOLIDAYS" ever be appropriate? We are using this method for fixing and payment dates, which would always need to be good business days.
If the input contains a days adjustment but no accompanying calendar it might be better to throw an exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be tackled separately
String indexStr = indexOpt.get(); | ||
Optional<FloatingRateName> frnOpt = FloatingRateName.extendedEnum().find(indexStr); | ||
if (frnOpt.isPresent()) { | ||
FloatingRateName frn = FloatingRateName.of(indexStr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems odd to not be using frnOpt.get() here?
private static RateCalculation parseFixedRateCalculation(CsvRow row, String leg, double fixedRate, Currency currency) { | ||
FixedRateCalculation.Builder builder = FixedRateCalculation.builder(); | ||
// basics | ||
builder.dayCount(DayCount.of(getValue(row, leg, DAY_COUNT_FIELD))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably involves too much work for the relative benefit, but would be nice to default the fixed DCF. There is a standard for each currency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be tackled separately
line++; | ||
} catch (RuntimeException ex) { | ||
failures.add( | ||
FailureItem.of(FailureReason.PARSING, ex, "CSV file trade could not be parsed at line {}: " + ex.getMessage(), line)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line count will not be incremented if exception is thrown.
case "FALSE": | ||
case "F": | ||
case "NO": | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might as well support "Y" and "N" options as well
} | ||
// dd/MM/yy | ||
// dd/MM/yyyy | ||
if (str.length() >= 6 && str.charAt(2) == '/' && str.charAt(5) == '/') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be 8 rather than 6
throw new IllegalArgumentException( | ||
"Unknown date format, must be formatted as " + | ||
"yyyy-MM-dd, yyyyMMdd, dd/MM/yyyy, yyyy/MM/dd, 'd-MMM-yyyy' or 'dMMMyyyy' but was: " + str, | ||
ex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dd/MM/yy not included in the exception message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exceptions are only including the recommended inputs, not everything we accept ;-)
|
||
//------------------------------------------------------------------------- | ||
public void coverage() { | ||
coverPrivateConstructor(FraTradeCsvLoader.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is FraTradeCsvLoader the only class tested here?
8b670f8
to
f030473
Compare
f030473
to
f46c4c9
Compare
Tenor tenor = tenorOpt.get(); | ||
LocalDate endDate = startDate.plus(tenor); | ||
SwapTrade trade = createSwap(info, conventionStr, startDate, endDate, buySell, notional, fixedRate, fxRateOpt); | ||
return adjustTrade(trade, rollCnvOpt, stubCnvOpt, firstRegStartDateOpt, lastRegEndDateOpt, dateCnv, dateCalOpt); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we update the javadoc (TradeCsvLoader) to reflect the fact that this combination is now supported?
Change column name Fix Javadoc
f440f89
to
63e91d4
Compare
FRA, Swap and Term Deposit.
All can be created from a convention or from individual fields.
Swaps do not support variable notional/rate/gearing/spread, but support almost all other features.